Fix crop phenology bugs with Gregorian calendar #2461
Draft
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
Includes a new test module with Gregorian calendar in which several tests are currently failing because DaysPastPlanting() uses days in THIS year.
Doing so because it complicates the determination of "how many days were in the previous year." Was only used in CropPhenology(), and results should be identical without it.
* Resolves failures in test_CNPhenology_gregorian. * Contributes to ESCOMP#1595: Bugs in crop phenology when running with a Gregorian calendar (ESCOMP#1595).
Should make no difference in standard runs. Could make a difference (harvest one timestep earlier) in Gregorian runs, but only if growing seasons of 365 days are allowed, which is not the case with default parameters except in GDD-Generating runs.
Fails with Gregorian calendar in non-leap years after Feb. 28, because when get_calday() is called with something like March 25 (325), it's assumed to be in year 0, which is leap.
This ensures that, during Gregorian runs, the dates returned for sowing windows always are leap or non-leap according to whether the current year is.
samsrabin
commented
Apr 10, 2024
| @@ -2742,7 +2743,7 @@ function DaysPastPlanting(idop) | |||
| else | |||
| ! get_curr_days_per_year() or get_prev_days_per_year() would only differ in the last timestep | |||
| ! of the year, but in that case this line is not reached. | |||
| DaysPastPlanting = jday - idop + get_curr_days_per_year() | |||
| DaysPastPlanting = jday - idop + get_curr_days_per_year(offset = -365*int(secspday)) | |||
There was a problem hiding this comment.
- If called on Dec. 31 (NOT at end of day, as is currently tested) of a leap year, will
get_curr_days_per_year(offset = -365*int(secspday))return 365 (correct) or 366 (incorrect)?
samsrabin
added
science
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.
Description of changes
As @billsacks documented in #1595, there were two crop phenology bugs that could occur during leap years. This PR:
My latter solution is a bit more complicated than Bill's suggested solution of just adding 10000 to the read-in MMDD number. However, it has the (slight) advantage of always conforming to the specified date, rather than being shifted a day early in leap years. It's a very cheap operation as well, happening just once per year per crop type.
Specific notes
Contributors other than yourself, if any: @billsacks
CTSM Issues Fixed:
Are answers expected to change (and if so in what way)? Yes, in leap years.
Any User Interface Changes (namelist or namelist defaults changes)? No
Testing performed, if any:
Unit tests of the behaviors in question failed initially and now pass.
aux_clmagainstctsm5.1.dev175unexpectedly showed no diffs. This turned out to be because the only aux_clm test with a Gregorian calendar isSMS_Ly5_Mmpi-serial.1x1_smallvilleIA.IHistClm50BgcCropQianRs.izumi_gnu.clm-gregorian_cropMonthOutput, which only compares against the output from the...h2.1854-12-31-00000.ncfile, which isn't a leap year. We thus should probably add a test with a Gregorian calendar that tests outputs from a leap year.... but then again, it does simulate one leap year, even if that's not the year being compared. Why were there NO differences?
Remaining tasks