Fix velocity exchanges across tripolar seam #4487
Conversation
|
Here is a visualization for how tracer conservation is broken in the modified test. |
…ceananigans.jl into nl/larger-streamfunction-in-test
|
@navidcy Just a heads-up — we probably don’t want to merge this PR yet. The TripolarGrid ZStar tracer conservation tests are currently failing, because this PR modifies the test in a way so that it exposes a deeper underlying issue. We're tracking that underlying issue in #4488. Probably best to resolve the underlying issue first before proceeding with this merge. |
|
We can merge if we get the tests to pass though right?? |
Yes, I just put that label there because @navidcy already approved the PR! Feel free to remove it. |
|
For us "approve" means (good to merge IF tests pass) but we are maybe a little loose sometimes 😂 |
NoraLoose
changed the title
|
Indeed, my approval was on the basis that tests pass. |
| @@ -93,7 +93,6 @@ end | |||
| end | |||
| end | |||
|
|
|||
| @inbounds v[i, Ny, k] = ifelse(i > Nx ÷ 2, sign * v[i′, Ny + 1, k], v[i, Ny, k]) | |||
There was a problem hiding this comment.
@simone-silvestri I think we need this line. Did you remove it on purpose?
There was a problem hiding this comment.
Yes, why do we need this line? The v velocity is not duplicated across the seam as the tracer and u-velocity is
There was a problem hiding this comment.
You're probably right — I think I was just confused.
Since we call
_fill_north_halo!(i, k, grid, v, bc::ZBC, ::CFLocation, args...) = fold_north_center_face!(i, k, grid, bc.condition, v)across the full range of i (not just the first or second half of the i-range), the line that you removed was redundant. All good.
There was a problem hiding this comment.
Not sure I understand. @NoraLoose's sketch showed that v is duplicated. What is the additional information that negates it?
There was a problem hiding this comment.
Or ok, the point is actually that the right side of the seam is the prognostic field, and on the left side the cells at Ny+1 are not evolved. So therefore we have the opposite rule for v than for other fields, and we should be filling in the reverse direction (eg putting interior values from the right of the seam onto the left boundary).
There was a problem hiding this comment.
Or ok, the point is actually that the right side of the seam is the prognostic field, and on the left side the cells at Ny+1 are not evolved.
Why are the cells at Ny+1 on the left side not evolved? They are responsible for advecting tracers correctly into the row tracer[1:half_point, Nx], correct? Or are we overwriting these tracer values on the left side anyway? Looking at this line, I think we are overwriting the tracer values on the right side, not on the left side.
There was a problem hiding this comment.
Oh "evolved", not "involved"
There was a problem hiding this comment.
So therefore we have the opposite rule for v than for other fields, and we should be filling in the reverse direction (eg putting interior values from the right of the seam onto the left boundary).
I think we are always taking values from the ride side, and copying them over to the left side (possibly with a sign change).
There was a problem hiding this comment.
Basically, we compute tendencies only up to Ny, so the Ny+1 must be determined by filling halos
Co-authored-by: Navid C. Constantinou <[email protected]>
As @simone-silvestri noted in this comment, the streamfunction used to initialize the velocity field in our tracer conservation test on the tripolar grid had an incorrect magnitude, resulting in initial velocities on the order of 10⁻⁶ m/s. This PR corrects the streamfunction so that initial velocities are O(1) m/s.
With non-negligible initial velocities, tracer conservation breaks down, see failing test " TripolarGrid ZStar tracer conservation tests".