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

Halo inflation + restriction on halo size makes it impossible to run some problems #3622

Closed
glwagner opened this issue Jun 14, 2024 · 14 comments · Fixed by #3732
Closed

Halo inflation + restriction on halo size makes it impossible to run some problems #3622

glwagner opened this issue Jun 14, 2024 · 14 comments · Fixed by #3732
Assignees
Labels
bug 🐞 Even a perfect program still has bugs

Comments

@glwagner
Copy link
Member

glwagner commented Jun 14, 2024

For example,

using Oceananigans

grid = RectilinearGrid(size = (16, 1, 16),
                       x = (0, 1),
                       y = (0, 1),
                       z = (0, 1),
                       topology = (Periodic, Periodic, Bounded))

model = NonhydrostaticModel(; grid, advection = WENO(order=5))

errors with

julia> include("problem.jl")
┌ Warning: Inflating model grid halo size to (3, 3, 3) and recreating grid. Note that an ImmersedBoundaryGrid requires an extra halo point in all non-flat directions compared to a non-immersed boundary grid.
└ @ Oceananigans.Models.NonhydrostaticModels ~/.julia/packages/Oceananigans/OHYQj/src/Models/NonhydrostaticModels/nonhydrostatic_model.jl:223
ERROR: LoadError: ArgumentError: halo must be  size for coordinate y

Sad, because I didn't actually ask for a halo (3, 3, 3)! Manually setting things doesn't help:

using Oceananigans

grid = RectilinearGrid(size = (16, 1, 16),
                       halo = (3, 1, 3),
                       x = (0, 1),
                       y = (0, 1),
                       z = (0, 1),
                       topology = (Periodic, Periodic, Bounded))

model = NonhydrostaticModel(; grid, advection = WENO(order=5))

I guess the restriction on the halo size is supposed to be an optimization, but its not working in this case because I can't even set up my model. @navidcy

@glwagner glwagner added the bug 🐞 Even a perfect program still has bugs label Jun 14, 2024
@navidcy
Copy link
Collaborator

navidcy commented Jun 14, 2024

okie -- there is definitely something wrong there

@navidcy navidcy self-assigned this Jun 14, 2024
@glwagner
Copy link
Member Author

bump ran into this again today

@simone-silvestri
Copy link
Collaborator

simone-silvestri commented Aug 25, 2024

It is not an optimization; rather, to fill in periodic halos, a field must have a grid size larger than the halos (for other boundary conditions, only one is required).
@navidcy, you could probably look at how the periodic halos are filled and make sure that they work also if the dimension is smaller than the number of halos.
It might entail writing some ifelse in the fill periodic halo kernels.

For example:

@kernel function fill_periodic_west_and_east_halo!(c, ::Val{H}, N) where H
    j, k = @index(Global, NTuple)
    @unroll for i = 1:H
        @inbounds begin
            # Put some ifelse condition here to make sure that halos pick from the interior
            # and not other halos
            c[i, j, k]     = c[N+i, j, k] # west
            c[N+H+i, j, k] = c[H+i, j, k] # east
        end
    end
end

@glwagner
Copy link
Member Author

@simone-silvestri we just need to be able to write

using Oceananigans

grid = RectilinearGrid(size = (16, 1, 16),
                       halo = (3, 1, 3),
                       x = (0, 1),
                       y = (0, 1),
                       z = (0, 1),
                       topology = (Periodic, Periodic, Bounded))

@simone-silvestri
Copy link
Collaborator

simone-silvestri commented Aug 25, 2024

One option is the one above, other options that could achieve that results are:
2) Limit the advection scheme order based on the grid size
3) we write something similar to TracerAdvection for momentum to allow different advection schemes in different directions and use nothing in the periodic direction with one grid point

@simone-silvestri
Copy link
Collaborator

I like option 3. I could open a PR to tackle this issue if there is consensus in the solution

@glwagner
Copy link
Member Author

But then what would you do for 2 grid points?

@glwagner
Copy link
Member Author

Why not implement both 3 and 2?

@simone-silvestri
Copy link
Collaborator

Thinking about it again, I would prefer to stay away from 2 because it results in very implicit behavior. It is nice to be explicit about the scheme used.

@glwagner
Copy link
Member Author

We already do 2 near boundaries

@glwagner
Copy link
Member Author

3 alone doesn't solve the problem, because we still don't know what to do with 2 grid points.

@glwagner
Copy link
Member Author

glwagner commented Aug 26, 2024

The question is just what you want to do with things like

using Oceananigans

grid = RectilinearGrid(size = (16, 1, 16),
                       halo = (3, 1, 3),
                       x = (0, 1),
                       y = (0, 1),
                       z = (0, 1),
                       topology = (Periodic, Periodic, Bounded))

model = NonhydrostaticModel(; grid, advection = WENO(order=5))

and

using Oceananigans

grid = RectilinearGrid(size = (16, 2, 16),
                       halo = (3, 2, 3),
                       x = (0, 1),
                       y = (0, 1),
                       z = (0, 1),
                       topology = (Periodic, Periodic, Bounded))

model = NonhydrostaticModel(; grid, advection = WENO(order=5))

etc

@glwagner
Copy link
Member Author

I think we want to handle this automatically because for example WENO() has a default order. I feel this falls within our philosophy of "friendly" (not sure if @simone-silvestri agrees with this philosophy though 😆 )

@glwagner
Copy link
Member Author

The risk of doing something automatic is that in theory someone could think "oh, I only have 2 grid poits in the y direction, but I still expect to observe WENO advection in that direction". Will someone think that? It doesn't really make sense, with 2 grid points in a direction there won't be advection at all anyways. I don't think this is a big risk. Big risks are like, simulations are dead wrong when they present an illusion that makes them seem ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Even a perfect program still has bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants