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

Fix bug when using Lagrangian particles on Flat topologies #3550

Merged
merged 10 commits into from
Apr 22, 2024

Conversation

Jamie-Hilditch
Copy link
Contributor

Fixes #3545

  • Add truncate_fractional_indices to handle the truncation of nothing indices returned by fractional_indices for Flat dimensions. nothing indices are "truncated" to 1.
  • truncate_fractional_indices used in advect_particle and bounce_immersed_particle.
  • Add method of enforce_boundary_conditions for Flat boundaries. This method does not modify the particle position.

src/Fields/interpolate.jl Outdated Show resolved Hide resolved
@navidcy navidcy added the bug 🐞 Even a perfect program still has bugs label Apr 12, 2024
src/Fields/interpolate.jl Outdated Show resolved Hide resolved
Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

Looks great!

Should we add a test to make sure this doesn't break in the future? The low bar is just to test that interpolation doesn't error with a Flat direction (we don't have to test correctness, though we could do that too)

@Jamie-Hilditch
Copy link
Contributor Author

Should we add a test to make sure this doesn't break in the future? The low bar is just to test that interpolation doesn't error with a Flat direction (we don't have to test correctness, though we could do that too)

Yes, probably should. The current tests have a hard coded topology

function run_simple_particle_tracking_tests(arch, timestepper; vertically_stretched=false)
    topo = (Periodic, Periodic, Bounded)


    Nx = Ny = Nz = 5
...

We could add tests to check that simulations can run on different topologies without error. Alternatively, I don't see anything in the existing tests that would not be supported if one or more of the Periodic dimensions was Flat. So we could extent the existing tests to allow for different topologies.

@navidcy
Copy link
Collaborator

navidcy commented Apr 17, 2024

We can do something like this?

topologies = ((Periodic, Periodic, Bounded), (Periodic, Flat, Bounded))

for topo in topologies
    run_simple_particle_tracking_tests(arch, timestepper; vertically_stretched=false), topo = topo .... )

@Jamie-Hilditch
Copy link
Contributor Author

I think so. The only thing we'd have to add is a little bit of logic when constructing the grid.

@navidcy
Copy link
Collaborator

navidcy commented Apr 17, 2024

E.g.,

topologies = ((Periodic, Bounded, Bounded), (Periodic, Flat, Bounded))
sizes = ((5, 5, 5), (5, 5))

for (topo, size) in zip(topologies, sizes)
    @show topo
    @show size
end
topo = (Periodic, Bounded, Bounded)
size = (5, 5, 5)
topo = (Periodic, Flat, Bounded)
size = (5, 5)

@Jamie-Hilditch
Copy link
Contributor Author

Yeah that would work. It's a bit weird to pass y=(-1,1) to the grid constructor when y is Flat but it seems to be ignored. An alternative would be to pass the grid as an input to run_simple_particle_tracking_tests and move all the grid construction logic (including the stretched vertical grid) to separate functions.

@glwagner
Copy link
Member

Yeah that would work. It's a bit weird to pass y=(-1,1) to the grid constructor when y is Flat but it seems to be ignored. An alternative would be to pass the grid as an input to run_simple_particle_tracking_tests and move all the grid construction logic (including the stretched vertical grid) to separate functions.

Maybe better is to create a list of grids:

three_dim_grid = RectilinearGrid(arch, ...)
x_flat_grid = RectilinearGrid(arch, topology=(Flat, Periodic, Bounded), ...)

etc, then pass the grid into run_simple_particle_tracking_tests

@navidcy
Copy link
Collaborator

navidcy commented Apr 17, 2024

@glwagner's suggestion is cleaner!

@Jamie-Hilditch
Copy link
Contributor Author

Jamie-Hilditch commented Apr 18, 2024

How about something like this

archs = (CPU(),) # just to make this code run

timesteppers = (:QuasiAdamsBashforth2, :RungeKutta3)
y_topologies = (Periodic(), Flat())
vertical_grids = (uniform=(-1,1), stretched=[-1, -0.5, 0.0, 0.4, 0.7, 1])

lagrangian_particle_test_grid(arch, ::Periodic, z) = RectilinearGrid(arch; topology=(Periodic, Periodic, Bounded), size=(5, 5, 5), x=(-1, 1), y=(-1, 1), z)
lagrangian_particle_test_grid(arch, ::Flat, z) = RectilinearGrid(arch; topology=(Periodic, Flat, Bounded), size=(5, 5), x=(-1, 1), z)

for arch in archs, timestepper in timesteppers, y_topo in y_topologies, (z_grid_type, z) in pairs(vertical_grids)
    @info "  Testing Lagrangian particle tracking [$(typeof(arch)), $timestepper] with y $(typeof(y_topo)) on vertically $z_grid_type grid ..."
    grid = lagrangian_particle_test_grid(arch, y_topo, z)
    # run_simple_particle_tracking_tests(arch, grid, timestepper)
    @show grid
end

which would give us these 8 (per architecture) test cases

[ Info:   Testing Lagrangian particle tracking [CPU, QuasiAdamsBashforth2] with y Periodic on vertically uniform grid ...
grid = 5×5×5 RectilinearGrid{Float64, Periodic, Periodic, Bounded} on CPU with 3×3×3 halo
├── Periodic x  [-1.0, 1.0) regularly spaced with Δx=0.4
├── Periodic y  [-1.0, 1.0) regularly spaced with Δy=0.4
└── Bounded  z  [-1.0, 1.0] regularly spaced with Δz=0.4
[ Info:   Testing Lagrangian particle tracking [CPU, QuasiAdamsBashforth2] with y Periodic on vertically stretched grid ...
grid = 5×5×5 RectilinearGrid{Float64, Periodic, Periodic, Bounded} on CPU with 3×3×3 halo
├── Periodic x  [-1.0, 1.0) regularly spaced with Δx=0.4
├── Periodic y  [-1.0, 1.0) regularly spaced with Δy=0.4
└── Bounded  z  [-1.0, 1.0] variably spaced with min(Δz)=0.3, max(Δz)=0.5
[ Info:   Testing Lagrangian particle tracking [CPU, QuasiAdamsBashforth2] with y Flat on vertically uniform grid ...
grid = 5×1×5 RectilinearGrid{Float64, Periodic, Flat, Bounded} on CPU with 3×0×3 halo 
├── Periodic x  [-1.0, 1.0)      regularly spaced with Δx=0.4
├── Flat y
└── Bounded  z  [-1.0, 1.0]      regularly spaced with Δz=0.4
[ Info:   Testing Lagrangian particle tracking [CPU, QuasiAdamsBashforth2] with y Flat on vertically stretched grid ...
grid = 5×1×5 RectilinearGrid{Float64, Periodic, Flat, Bounded} on CPU with 3×0×3 halo
├── Periodic x  [-1.0, 1.0)      regularly spaced with Δx=0.4
├── Flat y
└── Bounded  z  [-1.0, 1.0]      variably spaced with min(Δz)=0.3, max(Δz)=0.5       
[ Info:   Testing Lagrangian particle tracking [CPU, RungeKutta3] with y Periodic on vertically uniform grid ...
grid = 5×5×5 RectilinearGrid{Float64, Periodic, Periodic, Bounded} on CPU with 3×3×3 halo
├── Periodic x  [-1.0, 1.0) regularly spaced with Δx=0.4
├── Periodic y  [-1.0, 1.0) regularly spaced with Δy=0.4
└── Bounded  z  [-1.0, 1.0] regularly spaced with Δz=0.4
[ Info:   Testing Lagrangian particle tracking [CPU, RungeKutta3] with y Periodic on vertically stretched grid ...
grid = 5×5×5 RectilinearGrid{Float64, Periodic, Periodic, Bounded} on CPU with 3×3×3 halo
├── Periodic x  [-1.0, 1.0) regularly spaced with Δx=0.4
├── Periodic y  [-1.0, 1.0) regularly spaced with Δy=0.4
└── Bounded  z  [-1.0, 1.0] variably spaced with min(Δz)=0.3, max(Δz)=0.5
[ Info:   Testing Lagrangian particle tracking [CPU, RungeKutta3] with y Flat on vertically uniform grid ...
grid = 5×1×5 RectilinearGrid{Float64, Periodic, Flat, Bounded} on CPU with 3×0×3 halo 
├── Periodic x  [-1.0, 1.0)      regularly spaced with Δx=0.4
├── Flat y
└── Bounded  z  [-1.0, 1.0]      regularly spaced with Δz=0.4
[ Info:   Testing Lagrangian particle tracking [CPU, RungeKutta3] with y Flat on vertically stretched grid ...
grid = 5×1×5 RectilinearGrid{Float64, Periodic, Flat, Bounded} on CPU with 3×0×3 halo 
├── Periodic x  [-1.0, 1.0)      regularly spaced with Δx=0.4
├── Flat y
└── Bounded  z  [-1.0, 1.0]      variably spaced with min(Δz)=0.3, max(Δz)=0.5 

@glwagner
Copy link
Member

that looks good, just make sure to add the missing space after a comma and also we prefer arch = tuple(CPU()) to arch = (CPU(),) (slightly easier to read)

Jamie-Hilditch and others added 2 commits April 21, 2024 14:47
Copy link
Collaborator

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

lgtm

@navidcy navidcy merged commit 024b83b into CliMA:main Apr 22, 2024
46 checks passed
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 this pull request may close these issues.

Lagrangian particles on Flat topology
3 participants