-
Notifications
You must be signed in to change notification settings - Fork 204
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
Allow divergent Lagrangian-mean velocity by subtracting Stokes drift before pressure correction #4013
Draft
glwagner
wants to merge
2
commits into
main
Choose a base branch
from
glw/divergent-lagrangian-mean
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Allow divergent Lagrangian-mean velocity by subtracting Stokes drift before pressure correction #4013
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 this subtracting/adding the Stokes drift at a specific depth from a depth-averaged current? Perhaps this is the kind of issue you were alluding to @qingli411
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.
Yes, I think here we should use cell-averaged Stokes drift and in the finite volume approximation of$\partial_z u^S$ we will need the values at specific locations. When Stokes drift is specified as an analytical function, we can make sure this is done consistently. But what I'm not sure about is that, if the Stokes drift cannot be specified as an analytical function, what values should we pass in? So maybe also keep the option to pass in the derivatives of Stokes drift?
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.
Hmm, it's a good idea to discuss the discretization. I think on a C-grid we probably want to have the Stokes drift at the same location as the velocity components. This would not be cell centers, but Face, Center, Center for u^S, Center, Face, Center for v^S, etc.
Probably, there is a correct discretization of the term$\tilde \omega \times u^L$ that conserves energy (where $\tilde \omega = \nabla \times u^S$ is the curl of the Stokes drift), similar to how there is a correct discretization of vorticity / Coriolis that conserves energy.
For example see how we do it for Coriolis on the sphere:
Oceananigans.jl/src/Coriolis/hydrostatic_spherical_coriolis.jl
Lines 112 to 119 in d8273e7
If we want to be able to naturally compute$\omega$ at the "C-grid vorticity locations" then I believe we want u^S at the same location as the velocity field? The three components of vorticity are at cff, fcf, and ffc for the x, y, z components of vorticity respectively.
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 we can build a simple example to demonstrate conservation of energy (which is$(u^L)^2 / 2$ ) which will show that we've done the discretization correctly.
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.
That's a fair point. Though it would still be possible to compute the Stokes drift numerically by integrating its derivatives, but maybe that's not so convenient.
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 pressure solver depends only on the divergence of the velocity field. If the Stokes drift gradients were provided, would we now need to specify every derivative since the terms needed for the pressure solver Eulerian-ization ($\partial_xu^s$ , $\partial_yv^s$ and $\partial_zw^s$ ) are distinct from the ones needed for the wave-averaged forces:
Oceananigans.jl/src/StokesDrifts.jl
Lines 153 to 167 in 0fc5708
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.
OK. Maybe asking the users to specify every derivative is too complicated. I agree that the easiest approach is to specify the Stokes drift as$u^S(x,y,z,t)$ and compute the required values and derivatives internally. The difference between the Stokes drift at a specific depth and a depth-averaged Stokes drift around that depth should be small if we have sufficient 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.
It's what we do currently 😂
We don't have to choose to do anything specific. We can keep the existing structure, and add a new one wherein users specify the Stokes drift only. Then the test of time shall arbitrate.
Mainly I think it could save us time and effort in the end to have just one implementation. Then we know that we are all using the same code and more likely to find bugs / benefit from improvements together.
I do think that either way, sufficient resolution renders either discretization similar. But it would be interesting to understand the implications of inadequate resolution too.
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 as @BrodiePearson pointed out we will need to specify three more spatial derivatives ($\partial_x u^s$ , $\partial_y v^s$ , and $\partial_z w^s$ )...
I agree. I was not saying we should ignore the differences. But I cannot think of a clean way to specify the Stokes drift. I think we need both the Stokes drift at a specific depth and the depth-averaged Stokes drift around that depth. Maybe asking users to specify both is another option? I was also thinking of an option to specify the wave spectrum, or a simplified one with less frequency bins as Brandon did in MOM6. But maybe these different options can be wrapped into another layer of interface in the StokesDrift module, or defined by users. Internally in Oceananigans, I guess we need$u^s$ and $v^s$ defined as depth-averaged values at the locations of $u$ and $v$ , $w^s$ defined at the location of $w$ , spatial derivatives should also be depth-averaged over a grid cell?
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.
We can compute the finite volume derivatives of the Stokes drift field in the same way that we compute the discrete finite volume derivatives of the prognostic velocity field. The simplest stencil is second-order accurate (it looks just like a finite difference stencil). You could be right though that the derivatives may be known with higher accuracy if users are allowed to supply them.
It wouldn't be too much work to put an interface where the needed derivatives may be optionally provided, and otherwise are computed with a finite volume method.