-
Notifications
You must be signed in to change notification settings - Fork 7
Add temporary global constants header file #288
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: develop
Are you sure you want to change the base?
Add temporary global constants header file #288
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 is mostly fine. I have one needed change and two suggestions that others can weigh in on:
- this fails Ctests for the three Tendency tests (TEND_SPHERE, TEND_PLANE, TEND_PLANE_SINGLE_PRECISION), probably due to roundoff-level changes in expected results. So we either need to look into that or modify the expected values appropriately
- it might be good to have a Pi2 = 2.0*Pi since that combination is very common
- should we declare these as Real rather than double? When running in single-precision mode, this would prevent any extra conversions/truncations during computations
|
I think defining them as Real is a good idea. I talked about this a bit with @rljacob. Once the universal constants file is merged to E3SM, we can include it in this header and type cast variables to the Omega Real type (and rename variables). |
20c1823 to
4581cfd
Compare
Thanks, @philipwjones and @sbrus89! Fixed the failed CTests. (I missed some hardcoded 9.8###s in those tests) Removing the "Draft" label from this PR now that the tests are fixed and the |
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.
Thanks for the changes. I verified it passes Ctests now. You could still propagate the new TwoPi into ManufacturedSolution, but I'll go ahead and approve this.
|
Looks good. Ran the ctests successfully on chrysalis. |
|
Question for the reviewers (@philipwjones, @sbrus89, @brian-oneill, @xylar, @mwarusz)... do we want to add things like |
|
The hard requirement is that any quantity shared with other components should use common constants so that any quantities are calculated consistently. If we want to be safe, we should probably use all available constants from the shared constants file but we'll need to be sure to use them correctly in Kokkos kernels. |
xylar
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 looks good to me. I think it will provide a great place for the shared constants to plug in in the near future.
|
Actually, now that E3SM-Project#7719 has been merged, do we want to make use of that here? Or do we want to follow up later? |
|
Presumably, the answer is later because this has already been stale awhile. |
sbrus89
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 looks great. Thanks @katsmith133!
|
I'm sorry to sneak in a last-minute request for this branch, but could we change: Omega/components/omega/src/ocn/Tendencies.cpp Line 182 in ba2f369
to use RhoSw instead of the Density0 config option?
See #312 |
|
@xylar I will update this PR so that its not stale, and then include E3SM-Project#7719 and your suggestion for |
86d3678 to
8605f34
Compare
|
@xylar please let me know if this covers the changes you requested: (1) changed it so that Tendencies.cpp uses And @sbrus89, @philipwjones, @brian-oneill, and @mwarusz, if you could all take one last look at this now that's I've made a few more changes, that would be great! It compiles and runs on pm-cpu and pm-gpu. |
|
@katsmith133, yes, these changes look excellent to me! |
|
Looks fine to me and verified Ctests on Frontier cpu/gpu |
Adds a global constants header file with many commonly used physical constants and conversion factors, and replaces uses of Gravity, Pi, and Pa2Dbar in several files. This is a temporary solution for a consolidated constants file (brough up in #179), to be replaced when a shared E3SM constants file becomes available. Values are a combination of what is in https://github.com/E3SM-Project/E3SM/blob/09b9b7e21e3d381eafa962e178a5951580e2fc74/share/util/shr_const_mod.F90 and https://github.com/E3SM-Project/Omega/blob/develop/components/mpas-ocean/src/shared/mpas_ocn_constants.F. Making the assumption that those are the agreed upon values we want to use (please correct me if I am wrong or if we want to add additional/remove unnecessary values).
Passes CTests on pm-cpu, pm-gpu, and frontier. Note: There is currently no documentation that goes with this PR and no new Ctests were added (though I am happy to add those if we think they are valuable).
Checklist