Modify ocean z-star ALE coordinate for inactive top cells #5254
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The results of the compass PR test suite on chrysalis with intel-mpi are: |
|
This feature was pulled from #5246, since the other code changes related to inactive top cells were BFB within the compass PR test suite. This PR addresses @mark-petersen 's comments #5246 (comment) and #5246 (comment). |
|
This PR introduces an allocatable array |
|
@cbegeman -- any idea who should review this? |
|
@mark-petersen and @xylar Would you like to review this PR since you were reviewers on #5246? |
|
Per my conversation with @mark-petersen, and since this PR is non-BFB anyhow, I added 2 commits to improve code correctness (cf8c705) and performance (cf8c705), which introduce similar diffs as noted in #5254 (comment). These fails indicate differences from baseline, not execution failures or decomp failures. |
|
Is this non-BFB for any case with the ocean model? |
|
@rljacob Yes, it is non-BFB for most ocean cases. |
|
Ok. This will have to wait until after the v2.1 tag probably next week. |
|
@xylar I made this bugfix 41fcf7c which affected several global test cases including This is ready for your and @mark-petersen's review |
|
@xylar and @mark-petersen, hope you both had a relaxing break! Just a reminder that this is awaiting your review. |
|
@cbegeman, I'm definitely happy to look at this. My understanding was that it couldn't go in until after the v2.1 tag gets made so I was holding off a little. |
|
@cbegeman, would it be easy to rebase this to make testing a little cleaner? |
cbegeman
force-pushed
the
ocn/modify-zstar-for-inactive-top-cells
branch
from
41fcf7c to
5ab5930
Compare
|
@xylar Done. I have not retested since the rebase but looked over the changes and there weren't any conflicts so I don't anticipate issues. |
|
Comparing with ifort debug on chrsalis, between this PR and master, I get: but max differences are all 1e-10 or less on the first timestep, except for one temperature measurement here I tested with intel optimized on chrysalis and see non-BFB behavior but at a similar or even smaller error: output:
|
mark-petersen
approved these changes
Jan 27, 2023
|
@mark-petersen Thank you for the review and testing! |
xylar
approved these changes
Jan 31, 2023
There was a problem hiding this comment.
@cbegeman, this looks really good to me. I'm approving based on your and @mark-petersen's through thorough testing.
Let's see how @jonbob's 10-year E3SM runs go as well, but I expect only tiny changes.
|
status: checking MPAS-Analysis output. |
|
status: MPAS-Analysis failed. Was fixed. Need to rerun tests. |
|
@xylar and @cbegeman -- I reran ten-year tests now that we've fixed the fill value mask issue. Comparison mpas-analysis output is at: 20230302.5254test.anvil |
|
Thanks @jonbob! These results look acceptable to me. |
|
Thanks @cbegeman -- please reach out to anyone else who might take a peek. @mark-petersen, you had reviewed this PR -- can you check and make sure these results look right to you? |
|
I agree, it would be good to have a look from @mark-petersen. |
|
@jonbob The link for the analysis, 20230302.5254test.anvil, is not working. Can you check that? Thanks. |
|
@mark-petersen -- lcrc is down for maintenance today |
|
Thanks. Yes, the two simulations are similar and appear to differ by phase only, not mean or magnitude of the variability. See, for example, the heat and transport plots: |
jonbob
added a commit
that referenced
this pull request
Mar 14, 2023
…next (PR #5254) Modify ocean z-star ALE coordinate for inactive top cells This PR increases the generalizability of vertical coordinates by relaxing the assumption that restingThickness spans ssh=0 to bottomDepth. As such, it is tangentially related to inactive top cells. The change affects cases where config_ALE_thickness_proportionality='restingThickness_times_weights' which is the default. [non-BFB]
|
Testing show expected baseline DIFFs, merged to next |
|
merged to master and expected DIFFs blessed |
|
Thanks @xylar, @mark-petersen, and @jonbob for your help with this PR! I'm looking forward to using the added flexibility in vertical coordinates for ice shelf cavities! |
Merged
jonbob
added a commit
that referenced
this pull request
Mar 28, 2023
jonbob
added a commit
that referenced
this pull request
Mar 29, 2023
xylar
added a commit
to xylar/compass
that referenced
this pull request
Apr 2, 2023
This merge updates the E3SM-Project submodule from [c9201a4](https://github.com/E3SM-Project/E3SM/tree/c9201a4f44540bb74cb3650e32bcbe27fb762ab1) to [b4d5b10600](https://github.com/E3SM-Project/E3SM/tree/b4d5b10600). This update includes the following MPAS-Ocean and MPAS-Frameworks PRs (check mark indicates bit-for-bit with previous PR in the list): - [ ] (ocn) E3SM-Project/E3SM#5254 - [ ] (fwk) E3SM-Project/E3SM#5490 - [ ] (ocn) E3SM-Project/E3SM#5541 - [ ] (fwk) E3SM-Project/E3SM#5498 - [ ] (ocn) E3SM-Project/E3SM#5564 - [ ] (ocn) E3SM-Project/E3SM#5553 - [ ] (ocn) E3SM-Project/E3SM#5519
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR increases the generalizability of vertical coordinates by relaxing the assumption that
restingThicknessspansssh=0tobottomDepth. As such, it is tangentially related to inactive top cells. The change affects cases whereconfig_ALE_thickness_proportionality='restingThickness_times_weights', which is the default.[non-BFB]