-
Notifications
You must be signed in to change notification settings - Fork 446
Adds sugarcane as a perennial crop based on Colmanetti et al. 2024 #7681
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
Adds sugarcane as a perennial crop based on Colmanetti et al. 2024 #7681
Conversation
|
Is this needed for any v3 simulations? |
|
@rljacob No, this is not needed for v3 simulations. |
|
@bbye You may have missed the email, or it may have gotten buried among other emails, so I'm sending you a reminder for the review of this PR. Thanks, Eva. |
|
@evasinha I think this looks good. I only have one suggestion, but it might be better suited to address in a future PR. The way sugarcane is grown (over multiple growing seasons if I understand correctly), means we only fertilize once at the beginning of the first year. We might want to update the fertilizer scheme to have multiple applications. However, if the sugarcane scheme works without the extra fertilizer, then perhaps we should hold off on changing it. |
|
@bbye Thank you for your review and feedback. Your comment about fertilizer application every year is valid. I checked how much and how often fertilizer was currently being applied, and found a minor miss in the code. Currently, fertilizer is applied based on the |
279be8d to
a98835c
Compare
|
@bbye I have added two new commits since your review based on our discussion in Slack. Can you please review them? Thanks. |
|
The changes look good to me. |
|
Thanks @bbye. |
|
i think this will wait till |
|
I don't think crops are on in watercycle cases so this could go in. It should go in if v3 HES/BGC sims plan to use it. What does @bpbond say? |
| rmat = 100.0_r8 * (onset_gdd(p)/gddmaturity(p)) | ||
|
|
||
| if (nyrs_crop_active(p) == 0) then ! Year 1 | ||
| aerial(p) = (1.0_r8 - arootf(ivt)) * min(1.0_r8, (1 - exp(-(rootd * 0.6 * rmat)))) |
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.
What the source or logic behind these seasonal curves? An explanatory comment might be useful here.
| af1 = max(0._r8, (rmat * sf1) - (ipf1 * sf1)) | ||
| af2 = max(0._r8, (1.0_r8 - exp(-((rmat * ecf2) - (ipf2 * ecf2)))) ) | ||
|
|
||
| astem(p) = min( (1.0_r8 - aleaff(ivt) - arootf(ivt)), (aerial(p) * max(af1, af2)) ) |
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.
Similarly, to my eyes a comment describing the reasoning (as opposed to the mathematics, which are straightforward) here would be useful
|
@rljacob HES doesn't currently have plans to use this in v3, as Eva says above, but having a perennial sugarcane crop would enable some interesting HES-oriented analyses wrt bioenergy etc. So I support this going in if possible. Thanks |
bpbond
left a 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.
Thanks @evasinha !
|
@evasinha Tried testing this branch on next, but the new parameter file appears to be missing from the input data server. I can add it if you point me to the file or if you added it, double check the permissions |
|
@peterdschwartz I have added the parameter file: |
|
tests were successful. should be able to merge today. |
…(PR #7681) This feature adds new parameterizations to represent the sugarcane crop in ELM. The implementation is based on sugarcane implementation in the Agricultural version of the Integrated Biosphere Simulator (Agro-IBIS) model (Cuadra et al., 2012 and Colmanetti et al., 2024). It is important to note that sugarcane is modeled as a perennial crop, and this requires modifying the crop and percrop parameters in the parameter file, along with accompanying changes in ELM. The model was calibrated using the methodology used in Sinha et al. 2022. [non-BFB] for Crops since the parameter file is updated.
|
merged to next |
|
@evasinha It seems that you only add the parameter data to chrysalis instead of the data server? The crop tests are failing on all the other machines. Apologies for the confusion about what I was asking. I will go ahead and add it to the data server. |
|
@peterdschwartz thanks for adding the parameter file to the data server. |
|
merged to master |
|
Thanks @peterdschwartz |
This feature adds new parameterizations to represent the sugarcane crop in ELM.
The implementation is based on sugarcane implementation in the Agricultural version of the Integrated Biosphere Simulator (Agro-IBIS) model (Cuadra et al., 2012 and Colmanetti et al., 2024). It is important to note that sugarcane is modeled as a perennial crop, and this requires modifying the
cropandpercropparameters in the parameter file, along with accompanying changes in ELM.The model was calibrated using the methodology used in Sinha et al. 2022.
[non BFB] for Crops since the parameter file is updated.