Skip to content
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

fix correction for no_leap calendar in tests #4723

Merged
merged 2 commits into from
Dec 20, 2024

Conversation

jedwards4b
Copy link
Contributor

Test suite:ERP_P64x2_Ld765.f10_f10_mg37.I2000Clm60BgcCrop.derecho_intel.clm-monthly and ERP_P64x2_Ld3285.f10_f10_mg37.I2000Clm60BgcCrop.derecho_intel.clm-monthly
Test baseline:
Test namelist changes:
Test status: bit for bit
Fixes #4722

User interface changes?:

Update gh-pages html (Y/N)?:

@jedwards4b jedwards4b requested a review from ekluzek December 19, 2024 16:14
@jedwards4b jedwards4b self-assigned this Dec 19, 2024
Copy link
Contributor

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

I ran aux_clm for CTSM in ctsm5.3.016 and it's all good. And the tests that failed compare exactly to ctsm5.3.015 as I would expect.

So this fixes this from my perspective. I can create a simple PR to bring this update in.

I do suggest adding some comments around what this section is doing so it's more obvious to future developers. And this really should have some unit testing added for it to test edge cases and longer scenarios. So I'm putting this as requesting changes for those two things, comments and some unittests.

@@ -225,14 +225,15 @@ def _set_restart_interval(
smon = startdatetime.month
ryr = restdatetime.year
rmon = restdatetime.month
while ryr > syr:
if rmon > 2 and calendar.isleap(ryr):
while ryr > syr and (ryr < restdatetime.year or rmon > 2):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this makes sense here. It's pretty dense and hard to understand though, so I suggest adding some comments here about what it's doing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

And actually because this is a bit dense, adding some unit testing around this would be good to do. And I wonder about edge cases for this as well especially dates around 2/28, that would be worth checking that it works correctly for that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this has to loop over every year from startdate to restdate I also wonder about how this would do for an especially long period of many years. Practically most likely we aren't going to do restart tests for especially long periods, but for T compsets for example multi-decade restart tests could make sense.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that unit tests would be ideal here - covering a few specific cases - if they aren't too hard to put in place. If they are hard to put in place for some reason, then having some comments explaining this logic feels like an acceptable alternative (actually, whether or not there are unit tests).

@jedwards4b
Copy link
Contributor Author

It turned out not to be too hard to add unit tests. Please feel free to try more cases.

Copy link
Contributor

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

The new commit is great and addresses my concerns. Thanks for the addition.

@jedwards4b jedwards4b merged commit 3336d89 into ESMCI:master Dec 20, 2024
7 checks passed
@jedwards4b jedwards4b deleted the fix/leapdatefortests branch December 20, 2024 20:09
@billsacks
Copy link
Member

Yes, thank you very much @jedwards4b !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exact Restart tests fail for 765 days
3 participants