Add vertical mixing tendencies#410
Conversation
TestingCTest unit tests
Polaris
|
|
Tested this branch using Here are results of
|
|
@katsmith133 ,
I will retest your branch with this test case since you've updated it. |
@hyungyukang, thanks for the plots! I doubt the updates I just made in #352 will result in any difference here, but doesn't hurt to check. Let me pull down this branch and take a look at things and see if anything pops out to me. |
7503bfb to
72ab187
Compare
fd950d6 to
5abe72c
Compare
|
Test results from |
f280be8 to
d272fbe
Compare
d272fbe to
f2a132f
Compare
|
@hyungyukang The PR doesn't include bottom drag in the implicit mixing, correct? So we will continue to use explicit bottom drag with implicit vmix? |
@cbegeman , Yes, the implicit bottom drag is not included in this PR. I plan to open a separate PR for it after this one is merged. |
f2a132f to
abbc5c2
Compare
katsmith133
left a comment
There was a problem hiding this comment.
My comments aren't super major, except for whether we want to split the vertical mixing stuff up in some way in anticipation of having multiple parameterizations.
| GradRichNumSmoothed = Array2DReal("GradRichNumSmoothed", Mesh->NCellsSize, | ||
| VCoord->NVertLayersP1); | ||
|
|
||
| // TODO: Temporary handling of TangentialVelocity |
There was a problem hiding this comment.
Is this temporary handling to be addressed in a later PR?
There was a problem hiding this comment.
Yes.
I haven’t identified an appropriate location for it yet, but I believe we will have a suitable place for those diagnostic computations by the time the split time stepper is implemented, if not sooner.
|
|
||
| } // end defineIOFields | ||
|
|
||
| // Apply implicit velocity vertical mixing |
There was a problem hiding this comment.
In MPAS-O we have several files for the vertical mixing. One that handles the below calculations and several that do the computation of the coefficients (either from KPP or something else). Do we want to follow that design choice here in Omega? I feel like it might be good, as we add more mixing parameterizations, to have them split off more? Thoughts @vanroekel?
There was a problem hiding this comment.
Yes I think so. This is what I'm doing in the forthcoming KPP PR. It's its own class in its own file.
| 0.5_Real * (VertVisc(JCell0, K + 1) + VertVisc(JCell1, K + 1)) / | ||
| (LocRhoSw * SpecVolEdgeTop); | ||
|
|
||
| G = DT * ViscAlphaEdgeTop / PseudoThickEdgeTop; |
There was a problem hiding this comment.
Do we want more descriptive names for these variables (D, H, and X)?
There was a problem hiding this comment.
I tried to follow the naming conventions used in TriDiagDiffSolver:
- https://docs.e3sm.org/Omega/Omega/devGuide/TridiagonalSolvers.html#diffusion-type-tridiagonal-system
- https://docs.e3sm.org/Omega/Omega/design/TridiagonalSolver.html#scratch-data-structs
I will consider whether more descriptive names would be appropriate. Otherwise, I can add more detailed comments to clarify the code.
There was a problem hiding this comment.
I added more detailed inline comments to VelVertMixSetup and TracerVertMixSetup to explain the tridiagonal diffusion system and the roles of the G, H, and X coefficients.
| (PseudoThickEdgeK + PseudoThickEdgeKp1); | ||
|
|
||
| const Real ViscAlphaEdgeTop = | ||
| 0.5_Real * (VertVisc(JCell0, K + 1) + VertVisc(JCell1, K + 1)) / |
There was a problem hiding this comment.
| 0.5_Real * (VertVisc(JCell0, K + 1) + VertVisc(JCell1, K + 1)) / | |
| 0.5_Real * (VertVisc(JCell0, K) + VertVisc(JCell1, K)) / |
I might be misremembering the indexing of the NVertLayersP1 variables. Would we be starting from K = MinLayerEdgeBot = 0 and then K + 1 corresponds to the VertVisc at the bottom of the top level? Should the index be changed or the name be changed to ViscAlphaEdgeBot?
There was a problem hiding this comment.
Yes, that’s correct. The code is written for the K loop ranging from MinLayerEdgeBot to MaxLayerEdgeTop. I double checked both the equation and the implementation, and the K + 1 index is correct. As you suggested, ViscAlphaEdgeTop should be renamed to ViscAlphaEdgeBot to reflect its actual location (other variables as well).
If I use K instead of K + 1 for VertVisc, the result differs from the Omega and MPAS-O results shown above:
| VertMix *VertMixInstance = VertMix::getInstance(); | ||
|
|
||
| if (!EosInstance) { | ||
| LOG_WARN("Eos has not been initialized. Skipping calculation of " |
There was a problem hiding this comment.
Do we only want a warning here? Would it make more sense to do this check once at init and fail the model if this happens?
There was a problem hiding this comment.
Thanks for catching that. That makes much more sense.
I updated the code so that the VertMix tendency is validated once during initialization in Tendencies.cpp rather than at every time step. Also, the model will now abort if vertical mixing tendencies are enabled without both Eos and VertMix being initialized.
|
|
||
| if (!EosInstance) { | ||
| LOG_WARN("Eos has not been initialized. Skipping calculation of " | ||
| "PresGradZ tendency"); |
There was a problem hiding this comment.
should this be VertMix?
There was a problem hiding this comment.
The code has been removed. VertMix tendency now checks Eos only during initialization.
| VertMix *VertMixInstance = VertMix::getInstance(); | ||
|
|
||
| if (!EosInstance) { | ||
| LOG_WARN("Eos has not been initialized. Skipping calculation of " |
There was a problem hiding this comment.
again i think an error would be better.
There was a problem hiding this comment.
The code has been removed, and the model will now abort if vertical mixing tendencies are enabled without both Eos and VertMix being initialized.
| "PresGradZ tendency"); | ||
| } else if (!VertMixInstance) { | ||
| LOG_WARN("VertMix has not been initialized. Skipping calculation of " | ||
| "VelVertMix tendency"); |
There was a problem hiding this comment.
The code has been removed. VertMix tendency now checks VertMix only during initialization.
- Implemented implicit vertical mixing using TriDiagDiffSolver. - Added applyVertMixImplicit to VertMix. - Applied vertical mixing once at the end of each time step. - Note: Tangential velocity is temporarily handled in VertMix.
Renaming cell-interface variables from 'Top' to 'Bot'.
- Validate the VertMix tendency once at initialization instead of during each time step. - Abort the model if vertical mixing tendencies are enabled but Eos or VertMix has not been initialized.
- Improve the inline documentation in VertMixSetup
add9968 to
31bac0c
Compare
|
I ran into some CTest issues after rebasing, but they have been resolved. I am now running the Polaris Omega test suite. |
TestingCTest unit tests
Polaris
|
vanroekel
left a comment
There was a problem hiding this comment.
Approving based on visual inspection, developer testing, and my own testing with the single column test case.





This PR adds the velocity and tracer vertical mixing tendency terms. The implicit vertical mixing is solved by using
TriDiagDiffSolverand is applied once at the end of each time step.TriDiagDiffSolver.VertMixImplicittoVertMix.ComputeGradRichardsonNuminVertMix, is temporarily handled inVertMix.This PR includes #352 for testing purposes.
Checklist
Testingwith the following:have been run on and indicate that are all passing.
has passed, using the Polaris
e3sm_submodules/Omegabaseline-pfor both the baseline (Polarise3sm_submodules/Omega) and the PR build