-
Notifications
You must be signed in to change notification settings - Fork 312
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 get_curr_yearfrac and get_prev_yearfrac with Gregorian calendar #1593
Fix get_curr_yearfrac and get_prev_yearfrac with Gregorian calendar #1593
Conversation
Also add unit tests of get_prev_yearfrac with a Gregorian calendar. This required moving the call to unittest_timemgr_setup into each unit test in test_clm_time_manager.pf so that individual tests can determine whether to use a gregorian calendar. Resolves ESCOMP#1590
@olyson feel free to limit your review to the small change in clm_time_manager if you don't want to look through all the unit test changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me thanks!
The hack was documented as being needed for shr_orb_decl, but get_calday isn't used for this purpose. Removing this hack makes this function work correctly for leap years.
@olyson @ekluzek the commit I just pushed (c432b6d) is NOT yet the fix needed for @olyson 's issue: I first wanted to make the simpler change of removing a similar hack from get_calday, which is used in very limited places in the code and so it seems totally safe to remove the hack from there. The only place this is used is to convert crop planting dates in the format mmdd to a real-valued day-of-year. I did some unit testing (with some temporary tests that I deleted, because they didn't seem valuable long-term) to show that this change won't change answers in this use case, even for a planting date of 1231 (this translates to 366.0 which was left as is by the "hack" code). However, it did raise a different issue that I'll open an issue for shortly. |
This hack is needed when the result will be used for a call to shr_orb_decl, but should not be used in other situations. (There were some calls where the result was not directly used in a call to shr_orb_decl, but it looked like the result should stay consistent with other variables that are used directly in a call to shr_orb_decl.) This also adds some unit tests. The two unit tests that test Dec 31 on a leap year failed before making the changes to clm_time_manager in this commit.
@olyson I have a pushed a fix that I expect will solve your latest problem, as we discussed yesterday. Do you want to see if the changes in this branch indeed fix all of your problems now? I have tested the build but nothing more than that yet. @ekluzek do you want to take a quick look at these changes before I start final testing? Note that I ended up going with the solution of adding an optional argument to get_curr_calday. This is because I would otherwise have needed to make 8 identical changes to apply the hack to 2 variables in each cap plus 2 more in clm_initializeMod, and that felt like too much duplication. |
It feels like it would be best to keep get_calday consistent with get_curr_calday in this respect. Also add some unit tests of the hack in get_calday and get_curr_calday
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. The use of that argument "reuse_day_365_for_day_366 " makes the hack put into place more obvious. Hacks like that often cause problems if they aren't more obvious, and that's what happened here. It was OK previously, but because get_curr_calday was assumed to function correctly for all calendars thing built on it could break.
Sorry for the delay. The simulations I'm running only work with the PPE tag so I had to bring in your changes manually as SourceMods. I put in changes to clm_initializeMod.F90, clm_time_manager.F90, and lnd_comp_mct.F90 (I'm not using nuopc or lilac and I figured I didn't need the testing changes). |
On the last time step of the year, get_days_per_year would use the "current" date (i.e., time 0 of the next year) and so would give the number of days per year in the next year. But what we actually want is the number of days per year in the just-ending year; we achieve this by using an offset of -dtime in the call to get_days_per_year.
And rename get_days_per_year to get_curr_days_per_year to be explicit. This fixes the previous commit, where the wrong type was used for the offset argument, in addition to making the use more explicit. The behavior for AnnualFluxDribbler is as in the last commit, with a change from master. (The message from the last commit was the following, which remains effectively the same here, though with implementation details changed: On the last time step of the year, get_days_per_year would use the "current" date (i.e., time 0 of the next year) and so would give the number of days per year in the next year. But what we actually want is the number of days per year in the just-ending year; we achieve this by using an offset of -dtime in the call to get_days_per_year.) For other uses, I have not yet determined what is correct, but I am maintaining the previous behavior.
@ekluzek @olyson - I'm working on solving the remaining issue(s) with the Gregorian test. One issue is that there is at least one call to I'm planning to go through and see if there are other uses of |
I agree that you should use curr and prev, since that's what's currently being done. If we change the name for it, we should change it throughout clm_time_manager. And I think it's reasonably clear, so I don't know that we really need to do that wholesale change. But, adding the new function for prev does sound necessary to make this more explicit. It's especially more explicit than the hidden hack that was done previously. So I support your description of what you are doing. Thanks for working through this tricky and subtle aspect of the code! |
I also agree that although it's not ideal, we should use curr and prev for consistency. I think your comments will make it clear what the differences are. And again, thanks for your thorough examination of this! |
With the latest changes on this branch, the test Suspecting that there might be other, similar issues in other calls to
So the main changes that I'm pretty sure should be made are those under "Things for which we should probably be using a fixed constant". I am going to open a separate issue for this (update: this is now #1612). I think the changes needed for that will be relatively simple, so I'll take a crack at them this week, but I'd like to do that in a separate tag, partly so those changes can be reviewed separately and partly because they will be more answer-changing than the ones in this PR (for Gregorian cases, at least). |
I did a final pass through the code, looking for any other uses of |
Starting final testing now.... |
Description of changes
Fix get_curr_yearfrac and get_prev_yearfrac with Gregorian calendar.
Also add unit tests of get_prev_yearfrac with a Gregorian calendar.
Specific notes
Contributors other than yourself, if any: @olyson gets most of the credit here
CTSM Issues Fixed (include github issue #):
Are answers expected to change (and if so in what way)? YES, but very limited: From a search through the code, I think this will only impact CNDV cases when run with the Gregorian calendar. (The changed code is also used by the AnnualFluxDribbler, but only to set some values in gridcell-level balance checks; this can cause a run failure as in #1590 but I don't think it will actually cause any answer changes. The changed code is also used for time-interpolated prescribed dynamic landuse (dyn_var_time_interp_type), but we currently don't use that for any variables.) I think (but am not 100% sure) that these changes won't show up in any tests, so I'm marking this PR as bfb, since I think it will be bfb from a testing standpoint.
Any User Interface Changes (namelist or namelist defaults changes)? No
Testing performed, if any: Only unit tests so far