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

Bugs in crop phenology when running with a Gregorian calendar #1595

Open
billsacks opened this issue Jan 6, 2022 · 2 comments · May be fixed by #2461
Open

Bugs in crop phenology when running with a Gregorian calendar #1595

billsacks opened this issue Jan 6, 2022 · 2 comments · May be fixed by #2461
Assignees
Labels
bug something is working incorrectly priority: low Background task that doesn't need to be done right away. science Enhancement to or bug impacting science

Comments

@billsacks
Copy link
Member

Brief summary of bug

From some code inspection, I see what look like minor bugs in crop phenology when running with a Gregorian calendar.

General bug information

CTSM version you are using: latest master

Does this bug cause significantly incorrect results in the model's science? Maybe: I haven't thought carefully or investigated the magnitude of the impact. I suspect the impact is small, but I'm not positive about that.

Configurations affected: Runs with prognostic crops and a Gregorian calendar

Details of bug

I see two issues that look like bugs (though there may be others as well):

  1. Some crops have a max_SH_planting_date of 1231. Normally get_calday will turn this into the value 365. However, if the run uses a Gregorian calendar, then get_calday uses a year of 0 for this date (because the general format is yyyymmdd, so 1231 implicitly has a year of 0); year 0 is considered a leap year, so get_calday actually returns a value of 366 for these crops. This is true with or without the Gregorian calendar "hack" that I just removed in c432b6d (since the value is exactly 366.0, that hack wasn't getting triggered for this value). I'm not sure what the implications of this are, but it seems possible that planting gets skipped entirely in some circumstances on non-leap years.

  2. The calculation of days past planting:

    idpp = int(dayspyr) + jday - idop(p)

    uses the number of days per year in the current year. However, I think what's relevant here is the number of days per year in the previous year. (Imagine that it's now Jan 2, and planting happened on Dec. 30 of the previous year. Whether this year is a leap year is irrelevant; what matters for doing this day subtraction is whether last year was a leap year, and so whether Dec 30 was day of year 364 or 365 last year.) I haven't followed this through the code to see the impact of this issue, though my gut feeling is that it's probably relatively small.

@billsacks billsacks added bug something is working incorrectly tag: bug - impacts science labels Jan 6, 2022
@billsacks billsacks changed the title Minor(?) bugs in crop phenology when running with a Gregorian calendar Bugs in crop phenology when running with a Gregorian calendar Jan 7, 2022
@billsacks
Copy link
Member Author

Thinking about (1) more, I think it really may lead to certain crops sometimes not getting planted at all on non-leap years when running with a Gregorian calendar. I'd suggest a solution for (1) of: After reading in the mmdd planting dates, add 10000 before converting them to a real-valued day of year, so that they are treated as being in year 1 (a non-leap year) instead of year 0. This will mean that, in leap years, planting happens one day earlier than the specified date, but that seems acceptable, and much better than possibly not planting at all.

@billsacks billsacks added next this should get some attention in the next week or two. Normally each Thursday SE meeting. priority: low Background task that doesn't need to be done right away. and removed next this should get some attention in the next week or two. Normally each Thursday SE meeting. labels Jan 7, 2022
@samsrabin
Copy link
Collaborator

samsrabin commented Feb 12, 2024

Confirming that (2) is still an issue after my crop calendar work:

DaysPastPlanting = jday - idop + get_curr_days_per_year()

Another, possibly simpler workaround for (1) might be to set a maximum m[nx][NS]Hplantdate of 365 after conversion to day of year.

@samsrabin samsrabin self-assigned this Feb 12, 2024
samsrabin added a commit to samsrabin/CTSM that referenced this issue Feb 14, 2024
* Resolves failures in test_CNPhenology_gregorian.
* Contributes to ESCOMP#1595: Bugs in crop phenology when running with a Gregorian calendar (ESCOMP#1595).
@wwieder wwieder added this to the CESM3 milestone Apr 4, 2024
samsrabin added a commit to samsrabin/CTSM that referenced this issue Apr 4, 2024
* Resolves failures in test_CNPhenology_gregorian.
* Contributes to ESCOMP#1595: Bugs in crop phenology when running with a Gregorian calendar (ESCOMP#1595).
@samsrabin samsrabin linked a pull request Apr 7, 2024 that will close this issue
1 task
@samsrabin samsrabin added science Enhancement to or bug impacting science and removed bug - impacts science labels Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something is working incorrectly priority: low Background task that doesn't need to be done right away. science Enhancement to or bug impacting science
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants