EAMxx: cosp expected behavior #7652
Conversation
|
mahf708
force-pushed
the
mahf708/eamxx/cosp-expected-behavior
branch
2 times, most recently
from
7c539a9 to
df0c09f
Compare
mahf708
force-pushed
the
mahf708/eamxx/cosp-expected-behavior
branch
from
df0c09f to
2bcd6c6
Compare
|
@whannah1 @bartgol @AaronDonahue pls take a look at the last commit 94ee77a. I decided to add it because I wanted to simplify the amount of work being done here; there's really only one sunlit_mask, and it is a mask as it is defined, so we make all these fields (including aodvis) just point to it as their mask. So, recycling it. It seems to work well in my testing. After your review, I (with help from Ben) will do a few prod-like tests to ensure this is not going rogue. In my few simple tests, this seems to be working well... but somethings may show up out of nowhere, so we will test some more |
| @@ -133,6 +134,8 @@ TEST_CASE("aodvis") { | |||
| if(SCREAM_BFB_TESTING) { | |||
| REQUIRE(views_are_equal(aod_hf, aod_tf)); | |||
| } | |||
| // Ensure the masks are identical (should be pointing exactly to each other) | |||
| REQUIRE(views_are_equal(aod_mask, sunlit)); | |||
There was a problem hiding this comment.
If you want, you can be more strict: rather than checking that the two store the same values, you could/should verify that the two fields are the same, by doing REQUIRE(aod_mask.is_aliasing(sunlit));. This checks that the two fields point to the same data in memory, and that the two headers are aliases (which include the case where the two header ptr point to the same obj).
This would guard against accidental changes of the diagnostic class, where we create a separate field (e.g., setting the mask to be a clone of the sunlit field).
|
Baseline diff is expected. I will merge. Edit: merging to next only, as it will cause diffs. |
persist cosp-computed fields when cosp in not run, properly handle fill-value treatment for night indices, and update docs to reflect new behavior. [NBFB]
we are not ready, could you remove from next tmrw? I still would like to test this in an ne4 prod like test |
components/eamxx/cime_config/testdefs/testmods_dirs/eamxx/rad_cosp_async/shell_commands
Outdated
Show resolved
Hide resolved
mahf708
force-pushed
the
mahf708/eamxx/cosp-expected-behavior
branch
from
88d3532 to
00779f0
Compare
|
I verified this worked for my liking and testing, but I couldn't add a good enough test that covers everything. There are a few important caveats, two jump to my head right away:
These may trip up some users, but that's well beyond the scope of this PR, so as @bartgol recommended privately, we should merge this PR and worry about other stuff later. |
|
@bartgol can I ask for an exception regarding the linter or would you like me to make it happy? I can take care of the formatting later in a follow-up PR... |
Yeah, that's fine.
Are we ready now? Can I re-merge to next, or do I need to revert? |
|
Yes, note all the failing jobs including linter are expected (various issues, but they are all expected) And there's no real need to go through next since this only touches EAMxx side of things, but going through next is fine with me for extra caution, etc. |
Even the non-eamxx gh/ci test? Edit: nvm, I see. |
- Persist cosp-computed fields when cosp in not run - Properly handle fill-value treatment for night indices - Update docs to reflect new behavior [NBFB]
persist cosp-computed fields when cosp in not run, properly handle fill-value treatment for night indices, and update docs to reflect new behavior.
[NBFB]