(0.91.14) Extend Lagrangian advection to immersed grids and add tests #3765

tomchor · 2024-09-06T11:26:19Z

tomchor · 2024-09-06T11:30:23Z

src/Models/LagrangianParticleTracking/lagrangian_particle_advection.jl

+        # Left-right bounds of the previous, non-immersed cell
+        xᴿ, yᴿ, zᴿ = node(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f)
+        xᴸ, yᴸ, zᴸ = node(i⁻,     j⁻,     k⁻,     ibg, f, f, f)

-            Cʳ = restitution
-            x⁺ = enforce_boundary_conditions(Bounded(), x, xᴸ, xᴿ, Cʳ)
-            y⁺ = enforce_boundary_conditions(Bounded(), y, yᴸ, yᴿ, Cʳ)
-            z⁺ = enforce_boundary_conditions(Bounded(), z, zᴸ, zᴿ, Cʳ)
-
-        end
+        Cʳ = restitution
+        x⁺ = enforce_boundary_conditions(Bounded(), x, xᴸ, xᴿ, Cʳ)
+        y⁺ = enforce_boundary_conditions(Bounded(), y, yᴸ, yᴿ, Cʳ)
+        z⁺ = enforce_boundary_conditions(Bounded(), z, zᴸ, zᴿ, Cʳ)
+    else
+        x⁺, y⁺, z⁺ = x, y, z


This part of the algorithm was failing so I had to change it. However, I'm not familiar with the lagrangian advection algorithm so I'm sure if this is the right way to do things. Can someone double-check this part? From what I can tell, @Jamie-Hilditch and @jagoosw were the main authors of this algorithm, so maybe we should hear from them also.

It might be cleaner todo

if immersed_cell ... return ... else return x, y, z end

But otherwise I agree this looks like it is needed.

you can also use an ifelse syntax to avoid branching:

xb⁺ = enforce_boundary_conditions(Bounded(), x, xᴸ, xᴿ, Cʳ) yb⁺ = enforce_boundary_conditions(Bounded(), y, yᴸ, yᴿ, Cʳ) zb⁺ = enforce_boundary_conditions(Bounded(), z, zᴸ, zᴿ, Cʳ) immersed = immersed_cell(i, j, k, ibg) x⁺ = ifelse(immersed, xb⁺, x) y⁺ = ifelse(immersed, yb⁺, y) z⁺ = ifelse(immersed, zb⁺, z)

This part of the algorithm was failing so I had to change it. However, I'm not familiar with the lagrangian advection algorithm so I'm sure if this is the right way to do things. Can someone double-check this part? From what I can tell, @Jamie-Hilditch and @jagoosw were the main authors of this algorithm, so maybe we should hear from them also.

You've not really touched the couple of things I contributed so it looks good from my end.

src/Models/LagrangianParticleTracking/lagrangian_particle_advection.jl

…to tc/ibg-tracking

Jamie-Hilditch

Looks good to me!

src/Models/LagrangianParticleTracking/lagrangian_particle_advection.jl

tomchor · 2024-09-10T15:37:03Z

Looks good to me!

Let me know if there's anything else needed. If there are no other code suggestions this should be ready for merging.

Jamie-Hilditch · 2024-09-12T20:46:31Z

Let me know if there's anything else needed. If there are no other code suggestions this should be ready for merging.

I'm happy but this is one for @simone-silvestri, @jagoosw or someone else with the power to write.

tomchor · 2024-09-15T10:11:31Z

@simone-silvestri @jagoosw @glwagner could please I get a review from someone?

simone-silvestri · 2024-09-16T14:59:09Z

src/Models/LagrangianParticleTracking/lagrangian_particle_advection.jl

+    # Left bounds of the previous cell
+    xᴿ = xnode(i⁻ + 1, j, k, ibg, f, f, f)
+    yᴿ = ynode(i, j⁻ + 1, k, ibg, f, f, f)
+    zᴿ = znode(i, j, k⁻ + 1, ibg, f, f, f)

-            Cʳ = restitution
-            x⁺ = enforce_boundary_conditions(Bounded(), x, xᴸ, xᴿ, Cʳ)
-            y⁺ = enforce_boundary_conditions(Bounded(), y, yᴸ, yᴿ, Cʳ)
-            z⁺ = enforce_boundary_conditions(Bounded(), z, zᴸ, zᴿ, Cʳ)
+    # Right bounds of the previous cell
+    xᴸ = xnode(i⁻, j,  k,  ibg, f, f, f)
+    yᴸ = ynode(i,  j⁻, k,  ibg, f, f, f)
+    zᴸ = znode(i,  j,  k⁻, ibg, f, f, f)


This is quite different from how it was written before.
For example it might be that:
xnode(i⁻, j, k, ibg, f, f, f) != node(i⁻, j⁻, k⁻, ibg, f, f, f)[1]
(not necessarily for a rectilinear grid but possibly for a curvilinear one)
Was there a bug in the previous implementation?

Yes, there was a bug with the previous implementation and it needed re-writing. Specifically, it was failing for Flat dimensions iirc. I'm not familiar with how this is implemented in curvilinear coords, and I don't remember lagrangian tracking being done in them before. Can we keep the code as it currently is in the PR?

well, I understand why node(i⁻+1, j⁻+1, k⁻+1, ibg, f, f, f) would fail, but node(i⁻, j⁻, k⁻, ibg, f, f, f) should exist also for Flat topologies. Can we do this:

xᴸ, yᴸ, zᴸ = node(i⁻, j⁻, k⁻, ibg, f, f, f)

and keep the right boundaries as written in this PR?

The previous code fails not because of the +1 index in the flat dimension, but because it doesn't return anything for the flat dimension:

julia> grid_base = RectilinearGrid(topology = (Periodic, Flat, Periodic), size=(5, 5), extent=(1, 1)) 5×1×5 RectilinearGrid{Float64, Periodic, Flat, Periodic} on CPU with 3×0×3 halo ├── Periodic x ∈ [0.0, 1.0) regularly spaced with Δx=0.2 ├── Flat y └── Periodic z ∈ [-1.0, 0.0) regularly spaced with Δz=0.2 julia> node(2, 2, 2, grid_base, Center(), Center(), Center()) (0.3, -0.7)

So in these cases the statement xᴸ, yᴸ, zᴸ = node(i⁻, j⁻, k⁻, ibg, f, f, f) fails with

Got exception outside of a @test BoundsError: attempt to access Tuple{Float64, Float64} at index [3] Stacktrace: [1] indexed_iterate @ ./tuple.jl:92 [inlined] [2] bounce_immersed_particle @ ~/repos/Oceananigans.jl2/src/Models/LagrangianParticleTracking/lagrangian_particle_advection.jl:75 [inlined]

So, if I understand your comment correctly, we can just replace

xᴿ = xnode(i⁻ + 1, j, k, ibg, f, f, f) yᴿ = ynode(i, j⁻ + 1, k, ibg, f, f, f) zᴿ = znode(i, j, k⁻ + 1, ibg, f, f, f)

with

xᴿ = xnode(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f) yᴿ = ynode(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f) zᴿ = znode(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f)

I think that should be equivalent. Note that node() is just a shorthand for (xnode, ynode, znode)

Oceananigans.jl/src/Grids/nodes_and_spacings.jl

Lines 35 to 37 in 0f52a6a

@inline _node(i, j, k, grid, ℓx, ℓy, ℓz) = (ξnode(i, j, k, grid, ℓx, ℓy, ℓz),

ηnode(i, j, k, grid, ℓx, ℓy, ℓz),

rnode(i, j, k, grid, ℓx, ℓy, ℓz))

I think it's better to use ξnode than node[1] which is a hack and not as easy to understand either.

To clarify, I think there are at least 2 separate issues being discussed here.

The indices to be used when calling the location for the right indices. The code before this PR was node(i⁻+1, j⁻+1, k⁻+1, ibg, f, f, f), which I believe should translate to ξnode(i⁻+1, j⁻+1, k⁻+1, ibg, f, f, f) and the same for η and r, with the same indices (that's how the PR actually is). @simone-silvestri seems to think that the correct call should be (as per a code suggestion below)

xᴿ = xnode(i⁻ + 1, j⁻, k⁻, ibg, f, f, f) yᴿ = ynode(i⁻, j⁻ + 1, k⁻, ibg, f, f, f) zᴿ = znode(i⁻, j⁻, k⁻ + 1, ibg, f, f, f)

or perhaps with ξ, η and r, but importantly here is that the indices are different. @simone-silvestri, could you elaborate?

@glwagner is suggesting using ξnode (η and r) instead of xnode (and y and z). This is how it was in the code before this PR, but I think we actually want xnode and ynode since the advection is performed with velocities in length/time, no?

I don't know what's correct. Seems like there is a missing test... but maybe @simone-silvestri can say. We did use this code on the sphere I thought.

Ok, I am a bit confused which node should be the correct ``right'' node. I guess if there are tests now and the i⁻+1, j⁻+1, k⁻+1, ibg, f, f, f signature works, then the implementation is correct

To be fair, tests works regardless of which of the two signatures we're considering here, so whatever the difference is, the tests don't pick up on it. I think I'll keep the algorithm as close to as it was before this PR as possible. This includes using the i⁻+1, j⁻+1, k⁻+1, ibg, f, f, f signature. As for the xnode versus ξnode debate, I actually realized the existing code used xnode for the non-immersed part of the algorithm and ξnode for the immersed grid. Likely the latter was done in error, so I'll keep everything as xnode for now. We can revisit this is the future if we find evidence that this isn't the proper way to do things.

test/test_lagrangian_particle_tracking.jl

…to tc/ibg-tracking

simone-silvestri · 2024-09-16T23:30:14Z

src/Models/LagrangianParticleTracking/lagrangian_particle_advection.jl

+    xᴿ = xnode(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f)
+    yᴿ = ynode(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f)
+    zᴿ = znode(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f)


Suggested change

xᴿ = xnode(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f)

yᴿ = ynode(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f)

zᴿ = znode(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f)

xᴿ = xnode(i⁻ + 1, j⁻, k⁻, ibg, f, f, f)

yᴿ = ynode(i⁻, j⁻ + 1, k⁻, ibg, f, f, f)

zᴿ = znode(i⁻, j⁻, k⁻ + 1, ibg, f, f, f)

sorry if I am going back and forth, but I am now convinced this is the correct right bounds formulation, the left bounds look good

Are you sure about this? The original algorithm had this:

xᴿ, yᴿ, zᴿ = node(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f)

The node() call is equivalent to three independent calls to xnode, ynode and znode with the same arguments:

Oceananigans.jl/src/Grids/nodes_and_spacings.jl

Lines 35 to 37 in 0f52a6a

@inline _node(i, j, k, grid, ℓx, ℓy, ℓz) = (ξnode(i, j, k, grid, ℓx, ℓy, ℓz),

ηnode(i, j, k, grid, ℓx, ℓy, ℓz),

rnode(i, j, k, grid, ℓx, ℓy, ℓz))

So to me, the original algorithm should translate to:

xᴿ = xnode(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f) yᴿ = ynode(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f) zᴿ = znode(i⁻ + 1, j⁻ + 1, k⁻ + 1, ibg, f, f, f)

which is what this PR currently has implemented. Am I missing something?

Or maybe you're saying that the original algorithm was wrong to begin with?

jagoosw

LGTM! Sorry it took me so long to get to this

extend lagrangian advection to all AbstractGrids

4b72263

tomchor commented Sep 6, 2024

View reviewed changes

tomchor added 5 commits September 6, 2024 15:16

cleaner syntax

2334603

bugfix for immersed grids with flat dimensions

25d02a6

add tests with immersed boundary grid

c37be12

oops, wrong topology

5e28d4b

convert immersed function to boolean output

9634e51

Jamie-Hilditch reviewed Sep 6, 2024

View reviewed changes

src/Models/LagrangianParticleTracking/lagrangian_particle_advection.jl Outdated Show resolved Hide resolved

tomchor added 5 commits September 9, 2024 13:53

simplify

abff491

Merge branch 'main' into tc/ibg-tracking

e858e54

clean up logic

97b197f

Merge branch 'tc/ibg-tracking' of github.com:CliMA/Oceananigans.jl in…

54b1dcf

…to tc/ibg-tracking

bump patch version

8bcdef1

tomchor changed the title ~~Extend Lagrangian advection to immersed grids and add tests~~ (0.91.14) Extend Lagrangian advection to immersed grids and add tests Sep 9, 2024

tomchor requested review from jagoosw, simone-silvestri and Jamie-Hilditch September 9, 2024 12:21

Jamie-Hilditch reviewed Sep 9, 2024

View reviewed changes

src/Models/LagrangianParticleTracking/lagrangian_particle_advection.jl Show resolved Hide resolved

tomchor added 2 commits September 9, 2024 18:44

added docstring

81ff38f

Merge branch 'main' into tc/ibg-tracking

6ac566a

Merge branch 'main' into tc/ibg-tracking

c786eaa

simone-silvestri reviewed Sep 16, 2024

View reviewed changes

test/test_lagrangian_particle_tracking.jl Show resolved Hide resolved

tomchor added 2 commits September 16, 2024 20:22

correct indices

31bec4e

Merge branch 'tc/ibg-tracking' of github.com:CliMA/Oceananigans.jl in…

28171f6

…to tc/ibg-tracking

simone-silvestri reviewed Sep 16, 2024

View reviewed changes

simone-silvestri approved these changes Sep 16, 2024

View reviewed changes

tomchor added 8 commits September 17, 2024 22:17

add LatLonGrid to tests

1fd6833

Merge branch 'main' into tc/ibg-tracking

17a5f89

write speed differently to try and avoid using too much parameter space

9635d03

use only horizontal speed

a06faf1

fix docs and convert code to doctest

f590acb

add output to doctest

65e76c0

add filtering

0cfe076

Merge branch 'main' into tc/ibg-tracking

2c53428

jagoosw approved these changes Sep 21, 2024

View reviewed changes

tomchor merged commit 6649223 into main Sep 21, 2024
46 checks passed

tomchor deleted the tc/ibg-tracking branch September 21, 2024 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(0.91.14) Extend Lagrangian advection to immersed grids and add tests #3765

(0.91.14) Extend Lagrangian advection to immersed grids and add tests #3765

tomchor commented Sep 6, 2024

tomchor Sep 6, 2024

jagoosw Sep 6, 2024

simone-silvestri Sep 6, 2024 •

edited

Loading

Jamie-Hilditch Sep 6, 2024

Jamie-Hilditch left a comment

tomchor commented Sep 10, 2024

Jamie-Hilditch commented Sep 12, 2024

tomchor commented Sep 15, 2024

simone-silvestri Sep 16, 2024 •

edited

Loading

tomchor Sep 16, 2024

simone-silvestri Sep 16, 2024 •

edited

Loading

tomchor Sep 16, 2024

tomchor Sep 16, 2024

glwagner Sep 17, 2024

tomchor Sep 18, 2024

glwagner Sep 18, 2024

simone-silvestri Sep 18, 2024

tomchor Sep 18, 2024

simone-silvestri Sep 16, 2024

tomchor Sep 17, 2024

jagoosw left a comment

	@inline _node(i, j, k, grid, ℓx, ℓy, ℓz) = (ξnode(i, j, k, grid, ℓx, ℓy, ℓz),
	ηnode(i, j, k, grid, ℓx, ℓy, ℓz),
	rnode(i, j, k, grid, ℓx, ℓy, ℓz))

(0.91.14) Extend Lagrangian advection to immersed grids and add tests #3765

(0.91.14) Extend Lagrangian advection to immersed grids and add tests #3765

Conversation

tomchor commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simone-silvestri Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jamie-Hilditch left a comment

Choose a reason for hiding this comment

tomchor commented Sep 10, 2024

Jamie-Hilditch commented Sep 12, 2024

tomchor commented Sep 15, 2024

simone-silvestri Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simone-silvestri Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagoosw left a comment

Choose a reason for hiding this comment

simone-silvestri Sep 6, 2024 •

edited

Loading

simone-silvestri Sep 16, 2024 •

edited

Loading

simone-silvestri Sep 16, 2024 •

edited

Loading