search

ESCOMP / CTSM

Community Terrestrial Systems Model (includes the Community Land Model of CESM)
http://www.cesm.ucar.edu/models/cesm2.0/land/
Other
306 stars 308 forks source link

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

Open billsacks opened 2 years ago

billsacks commented 2 years ago

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 c432b6d0721dfb5b05b5bd417d64c0aaad8aff84 (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: https://github.com/ESCOMP/CTSM/blob/57175712e10e443f37818c81b8e14c956f28de34/src/biogeochem/CNPhenologyMod.F90#L2033

    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 commented 2 years ago

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.

samsrabin commented 7 months ago

Confirming that (2) is still an issue after my crop calendar work: https://github.com/ESCOMP/CTSM/blob/c030c2342dac53d30ed419aca262db8fa7213ee6/src/biogeochem/CNPhenologyMod.F90#L2749

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