Add nonhydrostatic test group #374
Conversation
|
For the solitary wave test case, the plot looks like this |
|
@scalandr, could you give me instructions on which E3SM branch is needed to go with this test case and how to compile it? I think you mentioned that PETSC is required. |
|
@xylar, the E3SM branch is this one: |
|
@scalandr, thanks very much for the instructions. When I try to build on Anvil, I'm seeing: I presume your testing has been without OpenMP so far but it seems like you will want to test with |
xylar
left a comment
There was a problem hiding this comment.
@scalandr, this is really amazing work!
My overall feeling is that there is some room to reduce the redundancy between the two test cases. That would be worth discussing together. But it seems like a lot of the InitialState and Forward classes are copies of one another so there should be a way to simplify things a bit. At least it seems worth considering.
Another major thing is that it would be nice to use the existing infrastructure for creating vertical coordinates rather than doing everything in each InitialState class. This might require adding your z coordinate in which only the top layer moves but I see that that coordinate doesn't get used by default in the current setup.
And finally I think each InitialState class would benefit a lot from taking advantage of xarray to avoid loops wherever possible.
| config_run_duration = '0001_16:30:00' | ||
| config_dt = '00:01:00' |
There was a problem hiding this comment.
This is a pretty long simulation (which is obviously fine). For regression testing to make sure the nonhydro code is working as expected, it would be great to also have a shorter (say 3 time step) version of the test cases that includes validation of several fields against a baseline simulation (if provided). This would be something to consider as a follow-up PR.
compass/ocean/tests/nonhydro/stratified_seiche/initial_state.py
Outdated
Show resolved
Hide resolved
compass/ocean/tests/nonhydro/stratified_seiche/initial_state.py
Outdated
Show resolved
Hide resolved
| for k in range(0, nVertLevels): | ||
| activeCells = k <= maxLevelCell | ||
| salinity[0, activeCells, k] = S0 | ||
| density[0, activeCells, k] = rho0 + (10.0/2)*(1.0 - \ | ||
| np.tanh((2/1)*np.arctanh(0.99)*(zMid[0, :, k] + \ | ||
| 0.5*maxDepth - 0.1*np.cos((np.pi/10)*xCell[:])))) | ||
| # T = Tref - (rho - rhoRef)/alpha | ||
| temperature[0, activeCells, k] = config_eos_linear_Tref \ | ||
| - (density[0, activeCells, k] - config_eos_linear_densityref) / \ | ||
| config_eos_linear_alpha |
There was a problem hiding this comment.
It would be better to avoid looping over k if we don't need to. This would have at least advantages:
- you would not need to pre-initialized the 3D variables above
- looping is nearly always slow in python
The computation of the vertical coordinate that I suggested above will add a ds.cellMask variable that I demonstrate here (equivalent to your activeCells). But in this test case, you have hard-coded that there is no bathymetry so this might be needlessly complicated. You might want to leave out the masking (all .where(ds.cellMask) calls).
| for k in range(0, nVertLevels): | |
| activeCells = k <= maxLevelCell | |
| salinity[0, activeCells, k] = S0 | |
| density[0, activeCells, k] = rho0 + (10.0/2)*(1.0 - \ | |
| np.tanh((2/1)*np.arctanh(0.99)*(zMid[0, :, k] + \ | |
| 0.5*maxDepth - 0.1*np.cos((np.pi/10)*xCell[:])))) | |
| # T = Tref - (rho - rhoRef)/alpha | |
| temperature[0, activeCells, k] = config_eos_linear_Tref \ | |
| - (density[0, activeCells, k] - config_eos_linear_densityref) / \ | |
| config_eos_linear_alpha | |
| ds['salinity'] = (S0*xarray.ones_like(ds.zMid)).where(ds.cellMask) | |
| ds['density'] =rho0 + (10.0/2)*(1.0 - \ | |
| np.tanh((2/1)*np.arctanh(0.99)*(ds.zMid + \ | |
| 0.5*maxDepth - 0.1*np.cos((np.pi/10)*ds.xCell)))) | |
| # T = Tref - (rho - rhoRef)/alpha | |
| ds['temperature'] = config_eos_linear_Tref \ | |
| - (ds.density - config_eos_linear_densityref) / \ | |
| config_eos_linear_alpha |
The density calculations seems to be really tricky to format in a reasonable way so maybe compute some intermediate variable to make it more readable?
There was a problem hiding this comment.
Changed it as suggested, but I have not put intermediate variables. If you think they are necessary for the code to be readable I'll create them.
| # initial velocity on edges | ||
| ds['normalVelocity'] = (('Time', 'nEdges', 'nVertLevels',), | ||
| np.zeros([1, nEdges, nVertLevels])) | ||
| normalVelocity = ds['normalVelocity'] | ||
| for iEdge in range(0, nEdges): | ||
| normalVelocity[0, iEdge, :] = u[0, iEdge, :] * math.cos(angleEdge[iEdge]) |
There was a problem hiding this comment.
This seems complicated, given that u is just all zeros (unless I missed something). I think the following would do it:
| # initial velocity on edges | |
| ds['normalVelocity'] = (('Time', 'nEdges', 'nVertLevels',), | |
| np.zeros([1, nEdges, nVertLevels])) | |
| normalVelocity = ds['normalVelocity'] | |
| for iEdge in range(0, nEdges): | |
| normalVelocity[0, iEdge, :] = u[0, iEdge, :] * math.cos(angleEdge[iEdge]) | |
| # initial velocity on edges | |
| ds['normalVelocity'] = (('Time', 'nEdges', 'nVertLevels',), | |
| np.zeros([1, nEdges, nVertLevels])) |
Do you have in mind to support non-zero u in the future?
There was a problem hiding this comment.
The lines
normalVelocity = ds['normalVelocity']
for iEdge in range(0, nEdges):
normalVelocity[0, iEdge, :] = u[0, iEdge, :] * math.cos(angleEdge[iEdge])
are indeed there for the more general case of a non-zero initial velocity. So, in that case, all you have to do is changing u.
There was a problem hiding this comment.
Okay, that sounds reasonable.
|
A few more things I forgot to include above. I am still running the tests. 1 hour on an Anvil interactive node was not enough. Update: the tests ran successfully with a longer allocation and took about 84 minutes together. I would like to add PETSC to the Spack build for compass and try running with that instead of @vanroekel's build. I think it would be important to make a PR to add PETSC before this goes in because otherwise no one can use it without special instructions. Finally, I think it will be necessary to get the |
|
@scalandr, thanks for starting on addressing my comments! That's great! Let me know if you have questions on the ones that remain. Also, let me know when you are ready for me to re-review and then fix up the PEP8 stuff (as we agreed). It would be great to meet in person whenever we are allowed to go back to the Lab. Please get in touch when that happens so we can arrange something. |
|
Hi Xylar, I was able to compile my nonhydro code with openMP and PETSc both on, so |
|
@scalandr, that sounds good. What we need to test is that code unrelated to nonhydro still compiles fine with |
|
@xylar, what do you mean by the "code unrelated to nonhydro"? What I mean when I say nonhydro code is the standard v2 MPAS-O code, plus a few more modules in the |
Right, I understand that.
That's not quite the same as what I mean. We need to run several standard E3SM and compass test cases with the current E3SM If you make a PR for |
|
One more thing, E3SM will require a branch that is named something like |
|
In the name, what should the spelling be, |
Either is fine.
No, that is only necessary if you aren't using your own fork of E3SM. If you're using your own fork, including your username is redundant. |
|
About openMP, if I use the most updated version of v2_nonhydro and do I get |
|
@scalandr, I think I see. We are using different compilers and MPI libraries, and yours aren't detecting the error that I'm seeing. Sorry about not being more specific. I just tried a few hours ago and I don't think any changes were made to the You can look at the contents of |
|
I am very close to having PETSC and Netlib-LAPACK supported in compass. This should allow you to stop using @vanroekel's script and also let you test with any of the 13 configurations supported in compass: https://mpas-dev.github.io/compass/latest/developers_guide/machines/index.html#supported-machines |
|
@scalandr, okay the compass issue is fixed. You should be able to try #374 (comment) |
|
@scalandr, that's perfect, that worked for me, too. If you want to make a PR to E3SM-Ocean-Discussion, I'm happy to do some more rigorous testing in the next week or 2 and post the results. You should also get someone who is familiar with the nonhydro work itself to review. Hopefully, the discussion will go quickly and you can make a PR to E3SM once everyone is happy. |
96a6c84 to
1410863
Compare
xylar
left a comment
There was a problem hiding this comment.
@scalandr, one more small thing to fix -- there are 4 docstrings that still refer to the baroclinic channel instead of Nonhydro. Please fetch the remote branch to get my commit that fixed the formatting of the code, hard reset, and then fix the 4 docstring, commit and push.
xylar
left a comment
There was a problem hiding this comment.
Thanks @scalandr! That looks great. I'll run the tests again tomorrow using your E3SM branch. Assuming they still run successfully, I'll approve.
As we agreed, we're going to make the unusual step of merging this before the associated changes are in E3SM so this can be the basis for testing PETSc configurations and so that other nonhydro test cases can be added to this test group in follow-up PRs.
|
@scalandr, can you let me know which is the best E3SM branch to test with? I was testing with https://github.com/scalandr/E3SM/tree/v2_nonhydro but it looks like https://github.com/scalandr/E3SM/tree/ocean/nonhydro was updated more recently. I'm going to try the second one for now. |
|
Use the second one please, i.e. |
xylar
left a comment
There was a problem hiding this comment.
Both test cases passed on Chrysalis with Intel and OpenMPI. I'm very pleased to add this capability!
|
@cbegeman, could you give this another look when you have a chance? @vanroekel, do you want to review as well? |
vanroekel
left a comment
There was a problem hiding this comment.
I took a look through the conversation and all the code and this looks great to me. Very happy to see this go in.
Approving on visual inspection and Xylar's testing.
cbegeman
left a comment
There was a problem hiding this comment.
@scalandr Great work on this! I just made some minor edits to the documentation. If you like, you can add the suggestions to a batch commit and then commit them directly from github.
I'm approving now based on your and Xylar's testing and successful tests of mine before the latest clean-up.
Thanks Carolyn! Co-authored-by: Carolyn Begeman <cbegeman@lanl.gov>
|
Hi Carolyn, I followed your suggestion, so I added your comments to a batch commit and then committed them directly from github. Thanks! |
|
Great, thanks everyone! |
Here is a new test group with two test cases to validate the new nonhydrostatic capabilites of MPAS-O. The tests consist of i) an internal solitary wave, and ii) an internal stratified seiche. For both tests, after building mesh and initial conditions, a hydrostatic and a nonhydrostatic simulation are run, and plots are made to show the differences between the two models.