-
Notifications
You must be signed in to change notification settings - Fork 321
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 misleading name of "gddplant" #1684
Conversation
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.
Thank you for taking on this cleanup. I'm happy with the changes you're making here – though I'd also like to make sure that @danicalombardozzi is happy with this. I do have some specific comments below, the most substantial of which is that I think we need to consider backwards compatibility with existing restart files (unless I'm misunderstanding things and in fact that's not needed).
- One other thing that's needed here is changing the following testmod to remove GDDPLANT and add HUI and GDDACCUM in its place:
'GDDPLANT', 'GDDTSOI', 'A5TMIN', 'A10TMIN' |
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.
Alright, this is looking really close now! I have a couple of small follow-ups.
to match what happens for VALUE. See ESCOMP#1684 (comment)
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 great now, thanks! I just want to get approval from Danica when she is available, and then we can do final testing of this PR.
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.
Thanks for your work on this @samsrabin! I very much appreciate the separation of HUI, used in the phenology calculations, and GDDACCUM, which accumulates the actual GDD. Two minor comments (questions, really).
|
||
call extract_accum_field ('GDDPLANT', rbufslp, nstep) | ||
call extract_accum_field ('HUI', rbufslp, nstep) |
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.
Does GDDACCUM also need to be added here? It seems that HUI and GDDACCUM are both added in most other places throughout the code, so I want to make sure it's not an oversight to leave it out here.
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.
I'll answer this one, since this is related to something I asked @samsrabin to change: GDDACCUM does not need to be updated here. However, the reason for this is subtle, and the fact that its absence caught your eye (thank you for your careful look that led to that!) suggests that there should probably be a comment here explaining why this is needed for HUI. Something like: "HUI needs this special update (which is non-standard for accumulation fields) because it can be changed outside of the accumulation routines (in CropPhenology), so the accumulation buffer needs to be reset when that happens." @samsrabin what do you think?
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.
Good idea, done (66abdbc).
@@ -333,7 +333,7 @@ subroutine CNSoyfix (bounds, num_soilc, filter_soilc, num_soilp, filter_soilp, & | |||
associate( & | |||
wf => waterdiagnosticbulk_inst%wf_col , & ! Input: [real(r8) (:) ] soil water as frac. of whc for top 0.5 m | |||
|
|||
hui => crop_inst%gddplant_patch , & ! Input: [real(r8) (:) ] gdd since planting (gddplant) | |||
hui => crop_inst%hui_patch , & ! Input: [real(r8) (:) ] patch heat unit index (growing degree-days) |
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.
Is HUI accumulated over a certain time period? GDD specifies that it's accumulated since planting, and I think it's the same for HUI. If so, does it make sense to add this description? Would something like "patch accumulated heat unit index (growing degree-days) since planting" be correct?
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.
You're right, a bit more info would be helpful. These descriptions now (10032f1) read (new text in bold), "crop patch heat unit index (growing degree-days); set to 0 at sowing and accumulated until harvest".
Thanks, both! |
Thanks @samsrabin! I think these changes look great and I'm happy to sign off on this! |
Description of changes
gddplant_patch
tohui_patch
(andGDDPLANT
toHUI
).gddaccum_patch
(outputGDDACCUM
).Specific notes
CTSM Issues Fixed: #1655
Are answers expected to change (and if so in what way)? No.
Any User Interface Changes (namelist or namelist defaults changes)? Yes. Output variable
GDDPLANT
(not included by default) is nowHUI
, and new outputGDDACCUM
has been added to track actual accumulation of GDDs since planting without any modifications such as occurs here:CTSM/src/biogeochem/CNPhenologyMod.F90
Lines 2044 to 2046 in d9ae4b4
Testing performed
As expected,
GDDACCUM
was never greater thanHUI
in any patch.Looking at 30 patches with discrepancies between
![patch547-temperate_corn](https://user-images.githubusercontent.com/10454527/157924458-e18940ae-301a-4909-a9a8-f1dfb1eecb4e.png)
GDDACCUM
andHUI
, the time series look as expected. E.g.:Diffs when testing with
SMS_D_Ld10_P72x1.f10_f10_mg37.IHistClm51BgcCrop.cheyenne_intel.clm-ciso_decStart.C.0311-123808ch
andSMS_P72x1_Lm25.f10_f10_mg37.I2000Clm51BgcCrop.cheyenne_intel.clm-monthly.C.0311-123808ch
are due to the rename ofGDDPLANT
toHUI
.