-
Notifications
You must be signed in to change notification settings - Fork 14
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 geostrophic convergence test (Williamson Test 2) #120
Conversation
This is based off of #119 and will be rebased after that gets merged. |
Thanks to @lconlon for starting out this test case with me in the Polaris Hackathon in May! |
aac2bcf
to
256f6df
Compare
TestingI have run all 4 geostrophic test cases on Chrysalis with Intel and OpenMPI. They ran successfully and produced the expected plots. The order of convergence for water-column thickness for the QU grids is really terrible -- 0.4. So that's discoraging for MPAS-Ocean but not an indication of a problem with the tests. The Icos meshes do much better -- about 1.6. |
2c32289
to
17bd0fc
Compare
I need to update this to use #126, so it will likely make sense to wait on reviewing this until after that PR has gone in. |
@sbrus89, sorry for the confusion on my part yesterday. I was too tired I guess and I missed that this was with a test merge. I have rebased and fixed thing up. Retesting now but I think you can give it another try when you have time. |
5e258d8
to
8088f7f
Compare
8088f7f
to
80b036b
Compare
@xylar, no problem. Yes I was testing with a local merge with |
I successfully tested the
I was having some performance problems with the |
@xylar, I'm seeing this error:
I looked in the |
@sbrus89, that does sound like a bug. Can you send me the commands you used to set up the 2 tasks? I want to make sure I can reproduce it. I always have been setting up both tasks together. |
@xylar - I did the following:
and then
|
@sbrus89, great, thanks! I'm looking into this now. Just as a tip to make your workflow more efficient, you may want to do:
and then:
This will set up a custom suite with all 4 tasks. But it doesn't seem to expose the problem you showed so I'm glad you did things the "more tedious" way. |
@sbrus89, I understand now what is going wrong and I don't have a good idea what to do about it yet. It isn't anything to do with this PR, it was caused by #125. When you separately set up 2 tasks that share the same config file, the first task will create a section, option and value with its [icos_geostrophic]
# A list of steps to include when running the icos_geostrophic task
# source: /gpfs/fs1/home/ac.xylar/e3sm_work/polaris/add-geostrophic-tests/polaris/setup.py
steps_to_run = icos_base_mesh_60km icos_init_60km icos_forward_60km icos_base_mesh_120km icos_init_120km icos_forward_120km icos_base_mesh_240km icos_init_240km icos_forward_240km icos_base_mesh_480km icos_init_480km icos_forward_480km analysis
[icos_geostrophic_with_viz]
# A list of steps to include when running the icos_geostrophic_with_viz task
# source: /gpfs/fs1/home/ac.xylar/e3sm_work/polaris/add-geostrophic-tests/polaris/setup.py
steps_to_run = icos_base_mesh_60km icos_init_60km icos_forward_60km icos_map_cell_60km icos_map_edge_60km icos_viz_60km icos_base_mesh_120km icos_init_120km icos_forward_120km icos_map_cell_120km icos_map_edge_120km icos_viz_120km icos_base_mesh_240km icos_init_240km icos_forward_240km icos_map_cell_240km icos_map_edge_240km icos_viz_240km icos_base_mesh_480km icos_init_480km icos_forward_480km icos_map_cell_480km icos_map_edge_480km icos_viz_480km analysis Presumably, the solution will be to include |
@xylar, I figured it wasn't related to this PR. I'll just review it without the fix. I'll try it by creating a custom suite as you suggested. Just a thought, it would be nice to be able to create custom suites with the test case names as opposed to numbers since those change as new tests are added. |
@sbrus89, that's a good idea and easy to implement. I'll make a PR shortly. |
@xylar, do you have any idea why I'd be seeing this:
It seems like something is going on with creating the path to the Registry. Part of the path to the worktree is being pre-pended to the full correct path: |
@sbrus89, hmm, that looks like a bug where a relative path didn't get turned into an absolute path as it should have before it got stored in the pickle file. I have never seen that before, so I would need to know exactly what your |
Sounds like it could be from not supplying |
I nearly always use |
@sbrus89, I'm not able to reproduce this with |
Oh, I take that back! I see it only in some tests and not others:
Again, not related to this PR but an important bug to fix. |
@sbrus89, I see what changed and why. It's complicated to explain (to do with fancy config parsing and automatically converting to absolute paths) but probably easy to fix. Thanks for bringing this to my attention. Can you proceed with using the |
@xylar - yes, will do. Sorry for the headache on this! |
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 to me. I successfully ran the new geostrophic cases on Perlmutter. Thanks for your work on this and for addressing these mostly unrelated issues @xylar!
Thanks so much, @sbrus89! And I really do appreciate you finding those issues, whether they are related or not. |
This merge adds the
geostrophic
convergence test case from Williamson et al. 1992. The test case starts out in geostrophic balance and ideally remains in steady state.The test case has similarities to
cosine_bell
so efforts have been made to share code, primarily theforward
step, between the two.Checklist
api.md
) has any new or modified class, method and/or functions listedTesting
comment in the PR documents testing used to verify the changes