Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CumulativeIntegral throws error when integrated over a reduced Field #4078

Open
tomchor opened this issue Feb 4, 2025 · 4 comments
Open

Comments

@tomchor
Copy link
Collaborator

tomchor commented Feb 4, 2025

It appears there's a bug with CumulativeIntegral when used on reduced Fields.

MWE:

julia> grid = RectilinearGrid(size=(4, 4, 4), extent=(1, 1, 1));

julia> c = CenterField(grid);

julia>= Field(Average(c, dims=(1, 2)));

julia> ∫c̄ = CumulativeIntegral(c̄, dims=3)
ERROR: UndefVarError: `Δzᵃᵃᶜ` not defined
Stacktrace:
 [1] metric_function
   @ /glade/work/tomasc/.julia/packages/Oceananigans/Dj1rO/src/AbstractOperations/grid_metrics.jl:31 [inlined]
 [2] Oceananigans.AbstractOperations.GridMetricOperation(L::Tuple{…}, metric::Function, grid::RectilinearGrid{…})
   @ Oceananigans.AbstractOperations /glade/work/tomasc/.julia/packages/Oceananigans/Dj1rO/src/AbstractOperations/grid_metrics.jl:78
 [3] *(Lc::Tuple{…}, a::Field{…}, m::typeof(Oceananigans.Operators.Δz))
   @ Oceananigans.AbstractOperations /glade/work/tomasc/.julia/packages/Oceananigans/Dj1rO/src/AbstractOperations/binary_operations.jl:122
 [4] *
   @ /glade/work/tomasc/.julia/packages/Oceananigans/Dj1rO/src/AbstractOperations/binary_operations.jl:126 [inlined]
 [5] (Accumulation{})(field::Field{…}; dims::Int64, reverse::Bool, condition::Nothing, mask::Int64)
   @ Oceananigans.AbstractOperations /glade/work/tomasc/.julia/packages/Oceananigans/Dj1rO/src/AbstractOperations/metric_field_reductions.jl:170
 [6] top-level scope
   @ REPL[20]:1
Some type information was truncated. Use `show(err)` to see complete types.

The same thing happens when using Integral instead of Average.

@glwagner
Copy link
Member

glwagner commented Feb 4, 2025

A little more direct MWE:

julia> c = Field{Nothing, Nothing, Center}(grid);

julia> ∫c = CumulativeIntegral(c, dims=3)
ERROR: UndefVarError: `Δzᵃᵃᶜ` not defined
Stacktrace:
 [1] metric_function
   @ ~/Projects/dev/Oceananigans.jl/src/AbstractOperations/grid_metrics.jl:31 [inlined]
 [2] Oceananigans.AbstractOperations.GridMetricOperation(L::Tuple{…}, metric::Function, grid::RectilinearGrid{…})
   @ Oceananigans.AbstractOperations ~/Projects/dev/Oceananigans.jl/src/AbstractOperations/grid_metrics.jl:78
 [3] *(Lc::Tuple{…}, a::Field{…}, m::typeof(Oceananigans.Operators.Δz))
   @ Oceananigans.AbstractOperations ~/Projects/dev/Oceananigans.jl/src/AbstractOperations/binary_operations.jl:122
 [4] *
   @ ~/Projects/dev/Oceananigans.jl/src/AbstractOperations/binary_operations.jl:126 [inlined]
 [5] (Accumulation{})(field::Field{…}; dims::Int64, reverse::Bool, condition::Nothing, mask::Int64)
   @ Oceananigans.AbstractOperations ~/Projects/dev/Oceananigans.jl/src/AbstractOperations/metric_field_reductions.jl:170
 [6] top-level scope
   @ REPL[73]:1
Some type information was truncated. Use `show(err)` to see complete types.

The issue I think is that because c has no location in x, y, we are looking for a metric that also has no location in x, y for the vertical integral. However, no such metric exists in general (for example if we are on an ImmersedBoundaryGrid with PartialCellBottom). What is the behavior that we would like to implement, in the case that Δz depends on x, y? We have to decide on an effective vertical metric for a reduced field.

@tomchor
Copy link
Collaborator Author

tomchor commented Feb 4, 2025

Yes, although for non immersed grids we don't have that problem and this should work, no?

Also, a quick note. While CumulativeIntegral(Field(Average(c, dims=(1, 2))), dims=3) fails, using CumulativeIntegral first and then averaging in the horizontal works:

julia> Average(Field(CumulativeIntegral(c, dims=3)), dims=(1, 2))
Average of 4×4×4 Field{Center, Center, Center} on RectilinearGrid on CPU over dims (1, 2)
└── operand: 4×4×4 Field{Center, Center, Center} on RectilinearGrid on CPU
    └── grid: 4×4×4 RectilinearGrid{Float64, Periodic, Periodic, Bounded} on CPU with 3×3×3 halo

@glwagner
Copy link
Member

glwagner commented Feb 4, 2025

Yes I think if you want to make some special cases for grids work then you can extend this constructor?

GridMetricOperation(L, metric, grid) = GridMetricOperation{L[1], L[2], L[3]}(metric_function(L, metric), grid)

@glwagner
Copy link
Member

glwagner commented Feb 5, 2025

Yes, although for non immersed grids we don't have that problem and this should work, no?

Also, a quick note. While CumulativeIntegral(Field(Average(c, dims=(1, 2))), dims=3) fails, using CumulativeIntegral first and then averaging in the horizontal works:

julia> Average(Field(CumulativeIntegral(c, dims=3)), dims=(1, 2))
Average of 4×4×4 Field{Center, Center, Center} on RectilinearGrid on CPU over dims (1, 2)
└── operand: 4×4×4 Field{Center, Center, Center} on RectilinearGrid on CPU
└── grid: 4×4×4 RectilinearGrid{Float64, Periodic, Periodic, Bounded} on CPU with 3×3×3 halo

Is this correct? I was playing around with CumulativeIntegral and it seems wrong that the location of the integral is the same as the location of the Field. I expected the location to flip, eg the same way it flips for a derivative. With that logic the cumulative integral of c would be at Face in the vertical.

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

No branches or pull requests

2 participants