-
Notifications
You must be signed in to change notification settings - Fork 37
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 overflow test group #501
Conversation
I intend to finish the documentation in the next couple days and update the checklist accordingly. |
afacf4a
to
fd1f725
Compare
@cbegeman, this is very exciting! It's a great test case to have because it's so visually clear how we're doing with mixing. I'll review as soon as I can. I'm trying to get some basic support PRs for both MPAS-Analysis and compass taken care of. |
Testing now with gnu on chrysalis. Here are my commands:
It looks like both cases: Using gnu debug, both cases end during the first time step with a segmentation fault. Here are more details:
@cbegeman can you confirm this behavior in your setup with a debug version? It would seem that the code is reading or writing out of bounds to get a segfault. But perhaps I made a simple mistake in the setup. If you reproduce this behavior, please try again with legacy compass, since this is just a straight translation. Thanks! |
@mark-petersen, I saw similar issues with Gnu in debug mode related to CVMix in the ISOMIP+ test case from the |
@mark-petersen, I created #513 for this and would appreciate your help investigating, but again I don't think it's related to this PR. |
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, most excellent work as always!
I have several formatting suggestions and related pieces of very minor clean up.
In addition, there are a few questions about possible changes to what are config options in overflow.cfg
, what are hard-coded "magic numbers" in code, and what are config options defined in code.
95334ea
to
a567a19
Compare
@xylar I have made all the changes you suggested. Thanks for your thorough review! |
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.
@mark-petersen Thank you so much for your review and testing! I appreciate you getting to this so quickly and testing out different machine/compiler combinations. |
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 told me in person that she retested this so I'm happy with the changes (and the comments about changes that will hold off for later).
@alicebarthel and @lconlon, could you let us know if you plan to review? I'll hold off on merging until we've heard back from you. |
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.
Passed visual inspection (more for myself to understand the process). I was able to reproduce the test on Anvil (intel-mpi). Thanks for the opportunity to watch and learn!
@alicebarthel Thanks for reviewing and testing! |
@lconlon I'm removing you as a reviewer but you're still welcome to give feedback if you like. |
@cbegeman, if you can rebase to fix the conflicts here, I'm happy to merge this. |
a567a19
to
8dee577
Compare
@cbegeman, I took the liberty of doing the rebase for you. It wasn't hard to resolve the conflicts. I tested the docs locally and they're fine. |
@xylar Thanks for the rebase and getting this in! |
Here we port the overflow test group from legacy compass. It features a density current flowing down a continental shelf. We include a default test at low resolution and a RPE test at higher resolution. The computation time for the RPE test was reduced from legacy by changing the resolution from 1km to 2km, the number of vertical levels from 100 to 50, and reducing the domain length from 240km to 200km. Only the sigma vertical coordinate is correctly initialized in MPAS-Ocean's init mode, so only that coordinate is tested here.
Checklist