Standard non-monotonic higher order horizontal advection copied from mpas-ocean to Omega. #316
Standard non-monotonic higher order horizontal advection copied from mpas-ocean to Omega. #316overfelt wants to merge 33 commits intoE3SM-Project:developfrom
Conversation
5ae40c2 to
1055c8f
Compare
|
This is the basic standard higher order horizontal advection scheme. MPAS-Ocean also has a monotonic FCT transport scheme that includes modifications to this basic scheme which is not included with this PR. Testing: |
|
@overfelt This is failing both the Horz Operators unit tests on Chrysalis and on Frontier (gpu only - cpu passes). The errors are memory errors - invalid pointer in free() on Chrysalis and inaccessible memory space (for array named XP) on Frontier gpu. So something isn't getting cleaned up correctly? I'll start reading through code to review... |
|
@philipwjones ,I only ran perlmutter gpu. I'll try setting up and running on Chrysalis gpu. Thanks. |
|
@overfelt, the tests also fail on perlmutter, both CPU and GPU, using the gnu compiler. Here are my instructions. Let me know if you can reproduce these errors. You will need to change to your own paths. perlmutter CPU:
perlmutter GPU:
|
35bf9b3 to
803b4a7
Compare
|
With the recent commit, this now passes CTests on Chrysalis and Frontier (cpu/gpu). Thanks @overfelt |
|
This passes CPU and GPU tests on perlmutter. Due to the merge of #314, this now has some conflicts to be resolved. Please rebase on the current head, and we will proceed with the review after the break. Thanks! |
9ec4b1a to
5eb5850
Compare
philipwjones
left a comment
There was a problem hiding this comment.
I have not done an exhaustive comparison to the paper or MPAS to verify the terms, but I did browse the code somewhat and this appears to be working fine based on convergence testing and passes all unit tests. Some of the code (esp HorzUtil) does not match Omega coding style, but I won't hold this up. At some point in the near future, I think we need to break up the monolithic tendency file.
|
I ran the ctests successfully on pm-cpu, pm-gpu, and Chrysalis. I agree with Phil, I think this is fine to move forward. Just FYI on Omega coding style, we're using PascalCase for variable names and camelCase for function names. This PR is just the standard horizontal advection algorithm so it's fine as is, but I think the monotonic advection algortihm is complex enough to warrant moving horizontal advection to it's own file, possibly as a separate module. |
|
@overfelt Glad to see this coming along! Let me know when this is ready to run Polaris tests and if you need any help getting those going |
|
What Polaris tests should I be running? I have only run the spherical cosine_bell test in my development.
|
It would be That includes the These convergence tests include some higher-resolutions meshes that have been hitting IO errors. If you want to bypass those, use the latest The order of the advection scheme tested will be whatever the default option is in Omega or you'll have to manually change it (until we update Polaris). If that's too much of a pain, let me know and I can automate it in a polaris branch. And then you can just visually examine the convergence plot and the state visualizations since the analysis PASS/FAIL will be based off of hard-coded orders of convergence (https://github.com/E3SM-Project/polaris/blob/bd87bf281986b9a28ad63370d0ad495140f4d84d/polaris/tasks/ocean/sphere_transport/analysis.py#L76-L79) |
|
These tests fail with the current head on perlmutter CPU and GPU with gnu: These are my commands
|
|
@overfelt Apologies. This change in my conservation fixes is what broke your tests: I haven't looked yet to check if there is an easy way to fix them. |
|
We do not want to be modifying the Earth radius or the coordinates on Omega. We need them to be fixed on the Polaris side. |
|
@mwarusz Your fix is the correct thing to do and the fix was trivial. Just had to copy your fix to the unit test so the check passes. |
components/omega/configs/Default.yml
Outdated
| BottomDragTendencyEnable: false | ||
| BottomDragCoeff: 0.0 | ||
| TracerHorzAdvTendencyEnable: true | ||
| TracerHorzAdvTendencyOrder: 1 |
There was a problem hiding this comment.
I think this config option belongs in the Advection section, and we should reserve the Tendencies section for options that are related to turning tendencies on/off. I know this hasn't been the case so far, so if folks have a different vision for organization, let me know.
There was a problem hiding this comment.
| TracerHorzAdvTendencyOrder: 1 | |
| HorzTracerFluxOrder: 1 |
In #328, the config option is VerticalTracerFluxOrder, so this change would make it consistent.
653f4e4 to
2da05b0
Compare
…uxOrder for consistentsy.
487843b to
06fab60
Compare
…ames Changes to match MPAS-O convergence rates in tracer advection tests
Checklist