-
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
Port ice_shelf_2d test cases #158
Conversation
@xylar This PR includes the ssh_adjustment framework I added. Please suggest changes if something different would be more convenient for isomip_plus. |
@xylar Also, when you have a chance, let me know what you think of this restart test approach. I'd like to do something similar for the baroclinic channel test. |
@cbegeman, thanks so much for this work! I'll take a look as soon as I can. |
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.
@cbegeman, what excellent work, as usual! I have some fairly minor suggestions focused on ssh adjustment for now, since I am interested in that shared functionality.
75721a6
to
77874a6
Compare
@xylar I think I've made all the changes you requested to the ssh_adjustment framework. Please let me know if there's anything else that is holding up your isomip_plus work. Otherwise, I'll move on to finishing up the rest of the tests in the group and docs and docstrings. |
@cbegeman, I haven't had a chance to look over the latest updates today but I do think I have what I need for |
Oh, thanks! I lost track of that one. |
The restart approach you have here looks great to me! |
c081067
to
d017f7b
Compare
TestingAll new single_layer and z-star tests have been run on chrys with intel, impi. |
@xylar I think this is all set to review. It took changing the number of vertical levels (from 20 to 50, though I didn't test values in between) to revive z-level (otherwise ssh_adjustment hangs). |
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.
@cbegeman, I'm sorry for not doing a thorough enough review the last time through. I noticed another issue in the SSH-adjustment framework that will need to be addressed (and maybe one we should discuss).
I also started testing the ice_shelf_2d
tasks themselves on Perlmutter but it's about to go down for maintenance.
@xylar When you have a chance, you can try the new setup_ssh_adjustment_steps approach with an ssh_forward class and an overridden compute_cell_count method. |
25ce071
to
7699544
Compare
@cbegeman, wow, that was fast work! I'll try this out again tomorrow. |
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 back to working on SSH adjustment in ISOMIP+. I ran into a few more things for us to chat about.
(to keep z-level case from crashing)
Generalize input filenames (instead of steps) resolution --> min_resolution
f105fac
to
8364eda
Compare
@xylar This is ready for your review. Thanks! |
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.
@cbegeman, very exciting to finally get this in!
I ran all the ice_shelf_2d
test cases on Chrysalis:
/lcrc/group/e3sm/ac.xylar/polaris_0.3/chrysalis/test_20240717/ice_shelf_2d
@xylar Thanks for your review and testing! |
This is largely a direct port of the ice_shelf_2d test cases from compass. The main changes are the following:
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