Skip to content

Fix broadcasting for operations that are not part of AbstractOperations.operations#5649

Open
simone-silvestri wants to merge 8 commits into
mainfrom
ss/only-broadcast-when-abstract-op
Open

Fix broadcasting for operations that are not part of AbstractOperations.operations#5649
simone-silvestri wants to merge 8 commits into
mainfrom
ss/only-broadcast-when-abstract-op

Conversation

@simone-silvestri

@simone-silvestri simone-silvestri commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator

For operations that are not registered as abstract operations, we do not want to spawn an abstract operation but just pass through the broadcast pipeline with a simple broadcast.
The example is:

using Oceananigans
grid = RectilinearGrid(size = (4, 4), topology = (Bounded, Bounded, Flat), x = (0, 1), y = (0, 1), halo = (1, 1))
field = CenterField(grid)
set!(field, 1.0)
Oceananigans.fill_halo_regions!(field)
field .= min.(field, 0.5) 

The last broadcast does not work in main but works when we bypass the abstract operations pipeline.
However, I have also added min as a @multiary operation (together with max) so that it can operate correctly on multiple fields.

Comment on lines +21 to +25
if Symbol(bc.f) ∈ operators
abstract_op = bc.f(loc, Tuple(broadcasted_to_abstract_operation(loc, grid, a) for a in bc.args)...)
return interpolate_operation(loc, abstract_op) # For "stubborn" BinaryOperations
else
return bc

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this produce incorrect results?

@glwagner glwagner self-requested a review June 1, 2026 16:17

@glwagner glwagner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think its a good idea to silently allow incorrect operations

@simone-silvestri

Copy link
Copy Markdown
Collaborator Author

what if we add a warning which explains how to deal with custom functions?

if !(Symbol(bc.f)  operators)
   @error "$(Symbol(bc.f)) is not registered as an AbstractOperation, see @unary, @binary and @multiary [ref...] to use $(Symbol(bc.f)) in broadcasting operations"
end

@glwagner

glwagner commented Jun 1, 2026

Copy link
Copy Markdown
Member

what if we add a warning which explains how to deal with custom functions?

if !(Symbol(bc.f)  operators)
   @error "$(Symbol(bc.f)) is not registered as an AbstractOperation, see @unary, @binary and @multiary [ref...] to use $(Symbol(bc.f)) in broadcasting operations"
end

I think that would be nice! We can also check whether we need interpolation. If we don't need interpolation, we can let it pass through (what I assumed would happen before I read the code)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants