-
Notifications
You must be signed in to change notification settings - Fork 219
Made ERI F1850C test work with gregorian calendar. #4899
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
Merged
+10
−3
Merged
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
14f3b15
Made ERI F1850C test work with gregorian calendar.
kdraeder 82240cf
Update eri.py
kdraeder 8b13390
fix black formatting
jedwards4b 0afcb5f
Use _cG instead of _CG for Gregorian calendar
billsacks aee737f
Merge branch 'ESMCI:master' into gregorian_ERI
kdraeder 5078565
Made ERI outfrq9s_leapday test work for HIST compsets
kdraeder 68d2197
Update test_scheduler.py
jedwards4b cc3d9c4
black reformat file
jedwards4b 33b54d8
Remove the consistency check of Gregorian and compset
kdraeder 82493b1
Merge remote-tracking branch 'esmci/master' into gregorian_ERI
billsacks File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
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 would prefer that this check not be here, for a few reasons:
First, I don't believe that there is a fundamental limitation of the system that you can't use the Gregorian calendar with spinup compsets. My impression from the discussion here is that there are problems in some components (e.g., CAM) when you try to use a Gregorian calendar in a spinup compset because of the way they have set up streams, but this is not a fundamental limitation of the system, and in the future we may want to fix that component behavior or have Gregorian spinup tests of a compset with components that don't have that issue – for example, I would expect a Gregorian X compset (coupler-test compset) test to pass currently; I haven't tried that, but if it fails I would like to be able to see that failure in whatever way it shows up rather than having CIME prevent me from running it at all.
An additional concern I have is that the mechanism for checking whether it's a spinup compset, by checking whether the compset name starts with a number, is - I believe - model-specific, not something that's prescribed by CIME, and so it doesn't feel like this logic belongs in CIME (though I could be wrong about that).
Also, if people feel it's important to have this check, then it should probably be done in a way that catches (probably more common) alternative ways of setting CALENDAR – i.e., by setting the CALENDAR XML variable after creating a case. And if this check is kept, I would prefer to see it moved to the component(s) where it is actually an issue. So, for example, this could be moved to the CAM build-namelist.
@kdraeder and/or @jedwards4b - if, after all this, you still feel it's best to have this check in CIME, I can accept it, but I then have a few specific questions / requests about the implementation of this check:
cGoption (further down in this file) rather than having thecGoption checked in two places?1in the{1,4}? It looks like this will match any compset that starts with a single digit... and if that's the intent then a simpler regex of just '\d' would be sufficient. I think the intent may be to only match a compset that starts with 4 digits, in which case I think the regex you want isr'\d{4}'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 agree that the assumption that if the compset name starts with a digit it's a spinup compset is misguided and this check should be removed. However I do not understand the utility or need to support a Gregorian calendar in a spinup compset.
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.
@billsacks Thanks for your insights!
Maybe there should be no gregorian modifier and it should be handled in the place(s) Bill suggests.
If it is handled in create_test, and if there's an actual limitation, there should be a check somewhere.
I agree that it should be clear, focused, and easy to remove.
I agree that there's not a fundamental limitation, but in addition to @JEdwards point, we discussed (above) the difficulty of specifying the correct (external forcing) namelist entries via the use_cases. It's beyond me to make the use_case aware of the calendar, so that it could specify suitable year(s) to extract from the forcing files (like it does with the grid). It could probably be done through a new testmod(s?). I haven't figured out why both use_cases and test_mods can alter the namelists, but one is user-selectable and the other isn't. That might answer the question of whether and how ERI for spinup compsets should handle gregorian.
If this were moved to the components' build-namelist, would the user have control over it in a way that's clear to people who run these tests?
If this limitation will stay for the time being, then:
I wasn't completely comfortable relying on the compset starting with 4 digits to flag spinup cases,
so if there are better solutions we should use them. But that seemed to be the convention.
Yes, the {1,4} should be {4} or {4,4}.
I tried to put this check in a place where _cG was already being handled, but it seemed that the compset was not available there. If it can be made available, then moving the check is good. Moving it into the component(s) would be fine too. It will take me a while to figure out how to do that.
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 also don't see enough scientific need for this to justify spending time on getting it working right now, but I don't think it should be fundamentally dis-allowed. Two things pop to mind: First, the use of a 365-day calendar by default is a model-specific thing, not a CIME thing; in principle, a model using CIME could decide that it wants to use a Gregorian calendar for all of its cases by default. (So I guess this is just a further argument that, if there is any logic about this, it should be in a component, not in CIME.) Second, I can at least see some theoretical reasons why someone might want to use a Gregorian calendar for a spinup run in CESM. One big one is for consistency: If you know you're going to use a Gregorian calendar for the later transient run, then you may want your spinup run to be consistent with that. I could see there being other reasons, too, that I / we aren't thinking of now.
I am generally in favor of adding error checking like this along with meaningful error messages to catch things that we know can't work together. I just want to see this localized to the place where the problem actually occurs so we don't end up preventing things that should actually work.
I don't have strong opinions one way or another on this. But I'd love to see this PR move towards resolution, so unless anyone else has strong-ish feelings, I'd vote for keeping this modifier in place as it is in this PR now.
Yes, agreed... but I feel this is a CAM limitation, not a CIME limitation, so if there is a check, it belongs in CAM's build-namelist.
Don't get me started on use_cases... the short answer is that I'm not a fan of them, but CAM uses them extensively, so... 🤷🏻♂️
I'm not sure what you're asking here; can you clarify?
There may be some additional options opened up if this check is moved into CAM's build-namelist, which I think is where it belongs. At the very least, it would at least be somewhat better to have this assumption about compset name be there rather than in CIME.
Ah, okay, thanks for clarifying.
I have appreciated your initiative in trying to get all of the needed changes in place yourself. That said, my suggestion at this point would be to take this check out of CIME and open an issue in CAM requesting a more clear error message when someone tries to use a GREGORIAN calendar in a non-transient case, and thus starting a discussion with the CAM developers about the best way to implement the check there.
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 confused myself here. I was thinking that the _cG might be moved from create_case to build-namelist, but we're talking about where the check should be, so _cG would stay in create_case
and be passed to build-namelist.
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.
It's possible that other components don't have this issue and can safely be run with a GREGORIAN calendar. Actually, now I'm confused about why you ran into issues with this in CAM: digging into the CDEPS code, it looks like streams should be set up to handle discrepancies between the model calendar and the streams calendar:
https://github.com/ESCOMP/CDEPS/blob/95e0bac4a723496b0da57d9d290c86e449c97238/streams/dshr_strdata_mod.F90#L807-L854
And indeed, I just successfully ran this CLM-only test:
ERI_D_Ld9.f10_f10_mg37.I1850Clm60Bgc.derecho_gnu.clm-gregorian(where I introduced a new test mod that simply set CALENDAR=GREGORIAN to avoid depending on the changes in this cime PR)This is a further argument to me that this error check should only exist in the component(s) that actually have an issue running this case.
Maybe the issue is that CAM doesn't use CDEPS's streams code in many places, but instead implements its own stream reading? From one spot-check, it looks like attempts were made even there to handle this discrepancy as well... though maybe this doesn't handle all cases, and maybe not all of CAM's streams have similar handling?:
https://github.com/kdraeder/CAM/blob/fc28fce1ff3f2a33840536e7128cb1eb8337b277/src/chemistry/utils/mo_flbc.F90#L130-L137
It would need to be a PR in CAM - you could ask over in your existing CAM PR whether they want you to include it in your existing CAM PR or open a new CAM PR. It's fine to merge this CIME PR without everything from #4811 being fully resolved, especially because some of the issues in #4811 are really outside of CIME. The CAM PR(s) would be a fix of a new issue, in CAM.
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.
Just following up from my last comment: I meant to add:
Based on what I see in code and my being able to run an 1850 Gregorian case in CLM, I think you should only worry about putting an error check about Gregorian+spinup compset into CAM, unless you have run into issues running this combination in other components. And even in CAM, it may be that this is a bug and that in fact this combination should work... I'd suggest discussing this with the CAM developers.
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've removed this inconsistency check from my clone by resetting back to @billsacks latest commit (0afcb5f).
> git reset --hard 0afcb5fb2My clone (and branch) is now 8 commits behind my github kdraeder fork. Git suggests that I update my clone, but I want to change my fork instead, to make 0afcb5f be the HEAD(?). Can I do that by pushing my clone to my fork? Or do I need to do something directly in my fork?
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.
My early 1850 test failed with:
Which led me to
It looked like I should just change these years to leap years (in the 1850_cam_lt use_case).
I didn't track down any potential coding causes of the failure; it seemed more likely that the namelist values were set incorrectly than that the code is insufficient.
I'll describe this more in the CAM issue that I open.
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.
First: I realized that my CTSM test wasn't a good one because it started Jan 1. I'm retrying with a test that starts in late Feb of 1852 and will post back here when I see those test results.
It can be very problematic to use
git reset --hardon a branch that has already been pushed, since pushing after that would be a destructive operation that rewrites history. As a general rule, you should only use that on your own work before pushing it to a place shared with other people, and even then it should be used with extreme caution because you can lose work with that command.Here's what you should do at this point:
(1) Update to the latest version:
git pull(2) Manually delete your recently-added check, commit and re-push. There are ways you could do this with git, but in this case, since there are multiple commits touching it and it's just a single self-contained block of code, it will be easier to just remove this manually and do an additional commit.
Sounds good. So it looks like the failure is in the module where I did find some handling of leap-year discrepancies, but maybe some other handling is needed... I haven't dug into this code very far.