Skip to content

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.

Sign up for GitHub

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
billsacks merged 10 commits into from
Dec 5, 2025
+10 −3
Merged

Made ERI F1850C test work with gregorian calendar. #4899

Show file tree
Hide file tree
Changes from 1 commit
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 Nov 18, 2025
82240cf
Update eri.py
kdraeder Nov 19, 2025
8b13390
fix black formatting
jedwards4b Nov 19, 2025
0afcb5f
Use _cG instead of _CG for Gregorian calendar
billsacks Nov 23, 2025
aee737f
Merge branch 'ESMCI:master' into gregorian_ERI
kdraeder Nov 25, 2025
5078565
Made ERI outfrq9s_leapday test work for HIST compsets
kdraeder Dec 3, 2025
68d2197
Update test_scheduler.py
jedwards4b Dec 3, 2025
cc3d9c4
black reformat file
jedwards4b Dec 3, 2025
33b54d8
Remove the consistency check of Gregorian and compset
kdraeder Dec 4, 2025
82493b1
Merge remote-tracking branch 'esmci/master' into gregorian_ERI
billsacks Dec 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CIME/SystemTests/eri.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ def run_phase(self):
start_1_year, start_1_month, start_1_day = [
int(item) for item in start_1.split("-")
]
start_2_year = start_1_year + 2
# KDR The default start date is a leap day, so 2-29 2 years later doesn't exist; run failure.
# start_2_year = start_1_year + 2
Copy link
Member

@billsacks billsacks Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this fix, but the comment is confusing, since I think it only applies to a specific case, not a general issue with the ERI test. What about changing it to something like:

Suggested change
# KDR The default start date is a leap day, so 2-29 2 years later doesn't exist; run failure.
# start_2_year = start_1_year + 2
# Change the year for the hybrid case to make sure the system can handle this
# change in year.
#
# Note: When using a Gregorian calendar, it can be important to have consistency
# between whether the two years are leap years or not because of how this test is
# set up; so change year by 4 so that these will generally be consistent.

(Note that I have also removed your initials and removed the commented-out old code.)

Copy link
Collaborator Author

@kdraeder kdraeder Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that comment is mostly left over from debugging.
Your suggestion looks good.
We could consider being more specific about "because of how this test is set up".
"because a testmod which tests the gregorian calendar starts the runs on leap day,
which must exist in all of the start years."

Copy link
Member

@billsacks billsacks Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your updated wording - my ambiguity came from my not fully understanding the situation.

Can you change this wording to your suggested version on top of mine?

start_2_year = start_1_year + 4
start_2 = "{:04d}-{:02d}-{:02d}".format(
start_2_year, start_1_month, start_1_day
)
Expand Down
8 changes: 4 additions & 4 deletions CIME/test_scheduler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,12 +674,12 @@ def _create_newcase_phase(self, test):
create_newcase_cmd += " --mpilib {}".format(mpilib)
logger.debug(" MPILIB set to {}".format(mpilib))
elif case_opt.startswith("N"):
expect(ncpl == 1, "Cannot combine _C and _N options")
expect(ncpl == 1, "Cannot combine _C# and _N options")
ninst = case_opt[1:]
create_newcase_cmd += " --ninst {}".format(ninst)
logger.debug(" NINST set to {}".format(ninst))
elif case_opt.startswith("C"):
expect(ninst == 1, "Cannot combine _C and _N options")
elif case_opt.startswith("C") and not case_opt.startswith("CG"):
expect(ninst == 1, "Cannot combine _C# and _N options")
Copy link
Member

@billsacks billsacks Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix, @kdraeder !

@kdraeder and @jedwards4b - What would you think about changing the CG modifier to instead be cG (lowercase G) to further remove some of this ambiguity? It doesn't look like we use any CG test modifiers in any CESM test list, so this shouldn't break anything... and it would be easier to change this now before we start getting tests with this modifier. This would be a departure from the current use of all uppercase as the first letter for a test modifier, so I don't love this inconsistency, but in my mind it's still better to have this disambiguation even if it means for some inconsistency in this respect. What do you both think?

Copy link
Contributor

@jedwards4b jedwards4b Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay by me.

Copy link
Collaborator Author

@kdraeder kdraeder Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you think about changing the CG modifier to instead be cG (lowercase G)

I'm guessing that you meant "lower case 'C'" not 'G'.
I wondered about that possibility, but didn't know how to see whether it would be disruptive,
so thanks for looking into it!
I agree that it would be clearer to not use _C for 2 modifiers.
How about using just _G (currently unused) for gregorian and _C# for the ensemble size?
That would preserve the use of only capitol letters for modifiers too.

Copy link
Member

@billsacks billsacks Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry, I meant lower case 'C', not 'G' - sorry for the confusion.

I also thought about using _G. I think that would be a reasonable option, though with these downsides / caveats:

  • I wondered if there might be a future desire to use some other calendar besides Gregorian; if so, that argues for keeping the "c" syntax rather than having completely separate options for each calendar. This was my main rationale that led me to the thought of keeping "_cG" rather than just "_G".
  • This line implies that "G" is a currently-accepted option, though from a quick look I can't see where it's used so maybe this is some historical code that isn't true anymore??? In any case, a little more digging should be done if we want to go with "_G":

cime/CIME/test_scheduler.py

Line 941 in 2f3f221

or opt.startswith("G") # handled in create_newcase

In all, I still lean towards "_cG", but don't feel strongly about it. So, @kdraeder, @jedwards4b or anyone else - if you have even moderately strong opinions about it, I'm happy to go with your feelings.

Copy link
Collaborator Author

@kdraeder kdraeder Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh, I missed that one.
I can see some value in being able to provide other calendars.
I don't have a deep background in what testing is or needs to be done,
so I don't have a strong opinion keeping the capital letter convention or adding modifiers.
A more thorough search leads me to believe that F, H, J, K, O, Q, S, T, U, W-Z are not being used.
_T for time? _K for kalender?

Copy link
Member

@billsacks billsacks Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still vote for lowercase "_c": I'd prefer something more mnemonic even if it breaks with the current uppercase convention.

@jedwards4b , @jgfouca @jasonb5 - any opinions on using a lowercase _c as a test modifier to specify calendar, replacing the current uppercase _C, which is currently used for two different purposes? It would be the first use of a lowercase test option, though I personally feel okay about this precedent as we get more and more test modifiers and start running out of letters.

Copy link
Contributor

@jgfouca jgfouca Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine with me.

Copy link
Member

@billsacks billsacks Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kdraeder is it okay with you if I push a change to your branch to change to using _c as the test modifier for calendar?

Copy link
Collaborator Author

@kdraeder kdraeder Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kdraeder is it okay with you if I push a change to your branch to change to using _c as the test modifier for calendar?

Yes, that should work well.
In which case the line that tests for _C and not _CG can be reverted.

ncpl = case_opt[1:]
create_newcase_cmd += " --ninst {} --multi-driver".format(ncpl)
logger.debug(" NCPL set to {}".format(ncpl))
Expand All @@ -698,7 +698,7 @@ def _create_newcase_phase(self, test):
if "--driver nuopc" in create_newcase_cmd or (
"--driver" not in create_newcase_cmd and driver == "nuopc"
):
expect(False, "_N option not supported by nuopc driver, use _C instead")
expect(False, "_N option not supported by nuopc driver, use _C# instead")

if test_mods is not None:
create_newcase_cmd += " --user-mods-dir "
Expand Down
Loading