-
Notifications
You must be signed in to change notification settings - Fork 6
Add vertical mixing coefficients module #310
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
Add vertical mixing coefficients module #310
Conversation
philipwjones
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.
This fails on Frontier GPU with a memory access error, so some array/variable may be missing or not scoped? I have some other comments spread below.
@philipwjones did the error provide any more information than that? At the moment my Frontier build is stopping at the cmake step (it keeps hanging at the |
|
@katsmith133 No additional info and I didn't have a chance to narrow it further, but I forgot to mention that it failed both EOS and VertMix unit tests, so it could point to something in the EOS additions. And I think this was with the AMD compiler - I can look into it further next week if no one gets to it first. |
Thanks @philipwjones! I am in a Deep Learning course this week, and probably will not get to it. So if you have time, that would be appreciated! If not, I can pick it back up next week. |
mwarusz
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 had a chance to look into the test failures of this PR on Frontier. There are issues with accessing host pointers on the device in EosTest.cpp and VertMixTest.cpp. My suggestion shows how to fix one loop, but similar changes need to be applied for all accesses to VCoord->ZMid and various Mesh->X arrays in these two tests.
|
Just pushed some changes to fix the frontier issue in EOS test driver (Vmix test driver mods will come soon). The commit also adds some additional EOS checks and cleans up some error handling, so folks should pull these changes if working on this PR |
mwarusz
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 have a few more comments and suggestions. There is a couple of serial vertical loops in this PR that I think could be easily done in parallel.
|
Ok, I pushed a new version of the vert mix test driver that fixes the Frontier issues, expands some error checks to full arrays, and cleans up a few things. This now passes ctests on Frontier. |
|
@philipwjones Thanks for the help in fixing the Frontier issues! @mwarusz, @cbegeman, and @vanroekel I believe I have addressed all of your comments. Please let me know if not. Thanks! |
philipwjones
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 fine with this now.
| Real PInt = | ||
| 0.5_Real * (Pressure(ICell, K) + Pressure(ICell, K - 1)); | ||
| Real SpInt = 0.5_Real * (SpecVol(ICell, K) + SpecVol(ICell, K - 1)); | ||
| Real AlphaInt = calcAlpha(SaInt, CtInt, PInt, SpInt); |
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 may want to put alpha and beta in member arrays as KPP will need these too and then we won't have to recalculate (I think).
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'll keep this as a note to change when we implement KPP. Thanks for pointing it out!
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.
Thanks for all your work on this @katsmith133! Most of my comments are suggestions for clarification (in the documentation or the naming). The key point to revise or resolve is whether
543fdb9 to
0dfdfdc
Compare
- added missing OMEGA_SCOPE statements - added some checks/tests to EOS driver - modified some error handling
- added needed OMEGA_SCOPE calls for some vars - update error handling - expanded error checks to full arrays - cleaned up some variable names
0dfdfdc to
57fb146
Compare
|
Passes all tests on perlmutter and frontier, CPU and GPU, including Test commands:
|
|
Thank you @katsmith133 and congratulations on the completion of this project. |
Add the
VertMixclass to Omega, which consists of:Methods to calculate the vertical diffusivity and viscosity using (for now just) the
PPoption, which can linearly add contributions from background, convective, and shear mixing based upon user choices in the config file. TheVertMixoptions and parameter values are set in the yaml config file. There is a dummyKPPoption in there now to set up the structure for adding it in the near future (I can remove if we think this is messy). This PR includes tests for each individual mixing process (background, convective, and shear), as well as one that linearly adds all of them together. Because both the convective and shear mixing require the Brunt Vaiasla frequency, this PR also adds the calculation ofBruntVaisalaFreqto the EOS class and adds corresponding tests. TheBruntVaisalaFreqis calculated in the way that was in MPAS-O if the Linear EOS is used (i.e. with linear coefficients and the derivative based upon changes inz) and is calculated in the same way the TEOS-10 package calculates it if the TEOS-10 EOS is used (i.e. with non-linear coefficients and the derivative based upon changes inp).Ran ctests successfully on pm-cpu, pm-gpu, and chrysalis.
Checklist