-
Notifications
You must be signed in to change notification settings - Fork 0
Add free-slip, partial-slip and no-slip momentum BCs #49
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
base: master
Are you sure you want to change the base?
Add free-slip, partial-slip and no-slip momentum BCs #49
Conversation
- config_wall_slip_factor multiplies rel. vorticity on boundary vertices, enabling: free-slip (factor=0), no-slip (factor=1), and partial-slip (0 < factor < 1) BCs.
- Fix-up boundaryVertex test and change default to keep current no-slip behaviour.
- Consider masked areas in cell-dual remapping; see Roullet et al, A Fast Monotone Discretization of the Rotating Shallow Water Equations, JAMES, 2021.
- Restructure loops to avoid array tmp., and update acc defines.
- Elide if through mask multiplication, and invert config option.
- Fix-up curl(u) loops.
|
@dengwirda, same questions as for #48: Is this ready to review? If so, who would you like to review it before it goes to E3SM? |
|
@xylar, let me know how you'd like the 'discussion' aspect of this to proceed --- this branch is ready to run, but I'd say that additional testing is required before review. |
|
@dengwirda, typically these PRs on E3SM-Ocean-Discussion only proceed if you ping specific people to indicate that you want their feedback. You can do that by adding them as a reviewer or just by |
|
@xylar please feel free to add extra reviewers and/or review yourself here. |
| !$acc present(circulation, relativeVorticity, areaTriangle, edgesOnVertex, & | ||
| !$acc maxLevelVertexBot, dcEdge, normalVelocity, edgeSignOnVertex, & | ||
| !$acc minLevelVertexTop) & | ||
| !$acc private(invAreaTri1, iVertex, iEdge, k, r_tmp) | ||
| !$acc present(circulation, relativeVorticity, edgesOnVertex, & | ||
| !$acc maxLevelVertexBot, dcEdge, normalVelocity, & | ||
| !$acc minLevelVertexTop, areaTriangle, edgeSignOnVertex) & | ||
| !$acc private(invAreaTri1, i, iEdge, k, r_tmp, wall_slip_factor) | ||
| #else | ||
| !$omp do schedule(runtime) private(invAreaTri1, i, iEdge, k, r_tmp) | ||
| !$omp do schedule(runtime) & | ||
| !$omp private(invAreaTri1, i, iEdge, k, r_tmp, wall_slip_factor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is boundaryVertex also needed in the present() arguments?
|
@dengwirda, your list of reviewers looks good to me. |
|
@dengwirda, I suggest you make one PR to E3SM to fix |
|
@dengwirda this looks like a good addition. When you compare the current code with this PR using |
|
@mark-petersen, I could be wrong but my understanding was that, because the area for vertices was being computed with full triangles rather than from the sum of the kites that actually exist, the differences will not be small and we will need to treat this as a (hopefully) non-climate-changing PR. That is why I was suggesting that we make the bug fix for the vertex area first and then the rest. |
| * kiteAreasOnVertex(i,iVertex) | ||
| end do | ||
| layerThicknessVertex = layerThicknessVertex*invAreaTri1 | ||
| layerThicknessVertex = layerThicknessVertex/areaDual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xylar I see what you are saying. This is the change where invAreaTri1 includes all three kites, while areaDual only includes the kites from active cells. This would be answer changing, and the changes in the loop above could go into a single PR.
| ! partial-slip: curl(u) reduced | ||
| circulation(k, iVertex) = circulation(k, iVertex) * & | ||
| (1.0_RKIND - wall_slip_factor * boundaryVertex(k, iVertex)) | ||
| relativeVorticity(k, iVertex) = circulation(k, iVertex) * invAreaTri1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this loop are what is needed to include wall_slip_factor. For this section, it is true that wall_slip_factor = 0.0 should only change the solution due to order of operations. For a second PR for just the addition of wall_slip_factor and these lines, it will be easier to show that only minor changes occurred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dengwirda, is there a reason to exchange loop order here? It seems like the order of operations issue could be avoided by applying the (1.0_RKIND - wall_slip_factor * boundaryVertex(k, iVertex)) factor in a separate loop k loop following the original do i = 1, vertexDegree loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the invAreaTri1 here needs a correction. The logic is similar to the areaDual below, but opposite. Currently, next to a full land boundary, areaTriangle(iVertex) does not include the non-existent cells. But beside a stair-step boundary below water where the neighbor exists, areaTriangle(iVertex) is the full area. So here invAreaTri1 is treating coastal boundaries versus underwater boundaries in a different way. Thinking about it, it seems each active edge should accumulate an associated area with it (the sub-triangle about that edge), and we should divide by that accumulated area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're suggesting exactly the same correction as what happens below with areaDual and that one is needed for the same reason -- handling topography.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, though, if the desired behavior is that the non-existent neighboring cells contribute zero vorticity, rather than that they don't contribut. And thus we would want to divide by the full triangle area, which is unfortunately not available for coastal vertices after culling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's accurate to think of the actual boundary as the one connecting the middle of each edge where normalVelocity = 0 (except when the edge is parallel to the boundary)?
I think the boundary really follows the boundary edges so it connects vertices, not edge centers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, then we would only want relativeVorticity = 0 at the vertices with all boundary edges and relativeVorticity != 0 for vertices with one non-boundary edge because those vertices "stick out" into the flow.
To me, the vertices with 2 boundary edges and one sticking out into the flow are precisely the ones that implement the choice of boundary conditions. Whether that free edge is allowed to contribute to the curl or not determines the boundary condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should be aiming for an implementation that would provide the same shear in the case where a straight boundary has a sawtooth shape (2 sides of hexagons) and one where the boundary has a snaking shape (3 sides of hexagons).
I think that's a really good question, and also whether either of these can be expected to approach the straight-wall solution at very high resolution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the boundary really follows the vertices, it seems unlikely that we could get both wall geometries to approach the no-slip solution. We could test the full dual area and the partial dual area (@xylar's suggestion for area lying in the active mesh) and see if either leads to better agreement. Given that, as @dengwirda mentions, most ocean models use free slip, we should leave the no-slip development there and move on to testing the free-slip case in realistic configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree that focusing too much on getting no-slip right is silly if free-slip is the way to go. Free slip seems much easier to get right.
|
@mark-petersen, @xylar, yes I think you're both right here --- the One reason I've grouped this work together here is that implicitly both changes relate to the way momentum boundary conditions are imposed. The |
|
@dengwirda, I see the logic in including them together. I think it will still be simpler in terms of how to frame these changes to the broader project if we first introduce a bug fix to the area weighting of layer thickness on vertices, knowing that the changes will not be small but that it's a bug. Then, we can still point to that PR when we introduce the other changes here but we would point out that the change in behavior after the bug fix is small as long as you are using the no-slip boundary condition. We would further point out that the "stealth" option to have free-slip or partial slip boundary conditions would be expected to introduce changes of a similar magnitude a and in a similar direction to the previous bug fix. If testing is sufficient at that point, we could also advocate for making something other than no-slip the default at that point. But I think it really will be necessary to introduce these changes in those 2 stages, rather than in one PR. |
mark-petersen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy for these changes to move to E3SM-Project. Thanks.
|
After over 2 years, I have had a chance to return to thinking about this. In the PR description, Darren states:
I do not think this is correct. Instead, I think the poorly named I therefore believe that the changes to the relative vorticity calculation in this PR are not needed. We could make a note that |
|
@xylar Thank you! I just wanted to document that @mark-petersen, myself, and Alaina Hogan are testing this branch and will also test a version without the dual mesh area changes. There is no need for anyone to make changes to this PR right now. |
|
@xylar and all re: the dual-area issue, I'd say that it's worth considering what happens throughout the water column (where land cells are present in the mesh but masked) rather than just at the surface (where cells are fully culled out). It looks like I have both the |
@dengwirda, ah, of course. That's a very good point. |
|
My preference for a path forward would be to move the We would use How does that sound? I also want to confirm that we should be determining active cells on a vertex using |
|
@cbegeman, sorry for waffling on this. I made a note about about why I think the area to use in the circulation is not the same as |
|
All --- a few extra thoughts:
|
|
Thanks for staying engaged with this, @dengwirda ! We'll look forward to your paper. I think we're stuck with our jagged boundaries for the time being but what you describe sounds really promising for a future improvement! |
|
That sounds like an interesting approach, @dengwirda. Please do share your paper when it is available! |
| !$acc present(circulation, relativeVorticity, edgesOnVertex, & | ||
| !$acc maxLevelVertexBot, dcEdge, normalVelocity, & | ||
| !$acc minLevelVertexTop, areaTriangle, edgeSignOnVertex) & | ||
| !$acc private(invAreaTri1, i, iEdge, k, r_tmp, wall_slip_factor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| !$acc private(invAreaTri1, i, iEdge, k, r_tmp, wall_slip_factor) | |
| !$acc private(invAreaTri1, i, iEdge, k, r_tmp) |
This change was made prior to testing. At least on some compilers, including wall_slip_factor in private results in it being assigned to 0 in the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes a bug of mine.
|
I'd like to migrate this over to an E3SM PR. If anyone has any objections or thinks further testing is needed before that review stage, let me know before Wed, Nov 5. |
|
Sorry for the slow response. Yes, go for it! |


















This PR updates the way horizontal momentum boundary conditions are computed --- highlighting that the current MPAS discretisation applies no-slip conditions, introducing new free-slip and partial-slip conditions, and cleaning-up the way cell-vertex remapping is handled across partially masked cells/duals in general.
No-slip and free-slip BCs can be expressed in terms of$\nabla \times \mathbf{u}$ on the boundaries --- no-slip conditions (the current implementation) correspond to a 'full' relative vorticity computed on boundary vertices, while free-slip conditions are imposed by setting $\nabla \times \mathbf{u} = 0$ on boundary vertices (corresponding to no induced torque when a free-stream velocity flows tangential to a wall). I've also introduced so-called 'partial-slip' conditions (see e.g. NEMO) as an interpolation between the no-slip and free-slip cases, leading to a reduced vorticity on boundaries. See e.g. Roullet and Gaillard for confirmation of how momentum BCs should be applied in a vector-invariant formulation.
A new
lateral_wallsnamelist section for BC settings is introduced, whereconfig_wall_slip_factorcontrols the amount of slip at horizontal walls (both coastlines at the surface and masked boundaries against bathymetry in general).config_wall_slip_factor = 0.0is a no-slip condition,config_wall_slip_factor = 1.0is a free-slip condition, and values between 0.0 and 1.0 correspond to partial-slip cases.No-slip (left) vs free-slip (right) behaviour can be seen in the channel case below (developed with @hetland) in which the initially uniform zonal flow generates strong vorticity sheets at the north/south boundaries in the no-slip case, while the flow remains quiescent in the free-slip case.
This PR also updates the way vorticity and layer-thickness is computed and remapped at partially masked cells and duals at boundaries, ensuring that the numerical stencils reflect the masking. In the case of computing potential vorticity$\frac{1}{h}(\nabla \times \mathbf{u} + f)$ it's necessary to remap layer thickness from cells to duals (triangles) using the intersecting cell-dual 'kite' areas. Currently, I feel we are not accounting for masked cells/duals correctly. Consider a case at a boundary in which one of the three cells surrounding a boundary vertex is masked, and where a spatially uniform flow with layer-thickness $h$ exists inside the domain.
In the current implementation the thicknesses from the two valid cells is remapped along with a 0 value for the masked cell. The result is divided by the full area of the dual (triangle) resulting in a vertex layer-thickness of only$\frac{2}{3} h$ , inconsistent with the spatially uniform interior thickness. An alternative approach implemented here is to accumulate only the unmasked area of the dual (triangle) and use this in the remapping, which will results the expected vertex thickness of $h$ .
I've tested both in channel and global standalone spin-ups. Results are non-BFB, with somewhat increased kinetic energy developing in global spin-ups with free-slip BCs due to the reduced dissipation compared to no-slip lateral boundaries.
Roullet, G. and Gaillard, T., 2022. A fast monotone discretization of the rotating shallow water equations. Journal of Advances in Modeling Earth Systems, 14(2), p.e2021MS002663.
https://www.nemo-ocean.eu/doc/node58.html