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

Adds climate driven planting date for crops #2999

Merged
merged 4 commits into from
Jul 16, 2019

Conversation

bbye
Copy link
Contributor

@bbye bbye commented Jun 13, 2019

Introduces a new method to calculate crop planting dates. Calculates the
temperature or precipitation seasonality of a grid and determines planting
date from temperature or precipitation thresholds. Therefore, the growing
season is either the warm or wet season. This allows crops to grow globally,
rather than in just the temperate latitudes.

A new module, CropMod is added to support some of the calculations and
will serve as a home of future crop model developments that are specific
to crops. The parameter file is not changed as there are no new parameters.

Also, fixes the dynamic landunit bug that won't allow crops to be planted
if they weren't on the grid cell previously.

Fixes #2962
[Non-BFB] - Non Bit-For-Bit (only for the crop model)

bbye added 2 commits May 31, 2019 17:11
Fixes the dynamic landunit bug that won't allow crops to be planted
if they weren't on the grid cell previously

[Non-BFB] - Non Bit-For-Bit (but only for the crop model)
Introduces a new method to calculate crop planting dates. Calculates the
temperature or precipitation seasonality of a grid and determines planting
date from temperature or precipitation thresholds. Therefore, the growing
season is either the warm or wet season. This allows crops to grow globally,
rather than in just the temperate latitudes.

A new module, CropMod is added to support some of the calculations and
will serve as a home of future crop model developments that are specific
to crops. The parameter file is not changed as there are no new parameters.

[Non-BFB] - Non Bit-For-Bit (only for the crop model)

See confluence for a more detailed description about these tags.
@bbye bbye added Land bug fix PR non-BFB PR makes roundoff changes to answers. labels Jun 13, 2019
@bbye bbye requested a review from bishtgautam June 13, 2019 23:46
@bbye
Copy link
Contributor Author

bbye commented Jun 13, 2019

The design document can be found at: https://acme-climate.atlassian.net/wiki/spaces/ED/pages/994934933/B5+Crop+Plant+Date+Design+Document

@bbye
Copy link
Contributor Author

bbye commented Jun 13, 2019

The baseline comparison tests for e3sm_land_developer all pass on anvil, with the exception of the error TPUTCOMP Error: Computation time increase > 10 pct from baseline, which I was told to ignore.

@rljacob rljacob added this to the v2.0alpha milestone Jun 14, 2019
@bishtgautam
Copy link
Contributor

@bbye Is the new physics covered by the existing crop test? Or, do we need to add an additional test for the new physics?

@bbye
Copy link
Contributor Author

bbye commented Jun 14, 2019

The current test will be fine. Once the surface datasets are updated I can create an additional test that would be a more thorough check, but that's not necessary.

Copy link
Contributor

@bishtgautam bishtgautam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good, but I have minor comments.

components/clm/src/biogeochem/CropMod.F90 Outdated Show resolved Hide resolved
components/clm/src/biogeochem/CropMod.F90 Show resolved Hide resolved
components/clm/src/biogeochem/CNPhenologyBeTRMod.F90 Outdated Show resolved Hide resolved
components/clm/src/biogeochem/CNPhenologyBeTRMod.F90 Outdated Show resolved Hide resolved
minor editrs to switch some local parameters to global parameters
for the crop plant date calculation

[Non-BFB] - Non Bit-For-Bit (only for crop model)
Copy link
Contributor

@bishtgautam bishtgautam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbye Thanks for the changes. The PR is good to go and is pending code-review approval.

@rljacob
Copy link
Member

rljacob commented Jun 19, 2019

I see an approved data on June 18 here: https://acme-climate.atlassian.net/wiki/spaces/ED/pages/994934933/B4+Crop+Plant+Date+Design+Document

@kvcalvin you approved it right?

@kvcalvin
Copy link

I did approve the design document. Is there something else I need to approve?

@bishtgautam
Copy link
Contributor

@rljacob, Do we also need to approval for verification, performance, and validation tasks before we integrate v2 PRs? see https://acme-climate.atlassian.net/wiki/spaces/ED/pages/995426324/B4+Crop+Plant+Date

jgfouca pushed a commit that referenced this pull request Jun 25, 2019
Port to new cgd system izumi

Test suite: scripts_regression_tests.py
Test baseline:
Test namelist changes:
Test status:
Fixes
User interface changes?:

Update gh-pages html (Y/N)?:

Code review:
@rljacob rljacob assigned jqyin and unassigned bishtgautam Jul 1, 2019
@rljacob
Copy link
Member

rljacob commented Jul 1, 2019

Reassigning to balance the integrator workload

@qzhu-lbl
Copy link
Contributor

qzhu-lbl commented Jul 9, 2019

@bbye energyflux_vars was added in subroutine "CNPhenology", in order to be used in "CropPlantDate". However, it seems that energyflux_vars is never used in "CropPlantDate"? Am I understanding this correctly? Is energyflux_vars redundant?

@bbye
Copy link
Contributor Author

bbye commented Jul 10, 2019

I think I see what happened. The variable I used from the energyflux_vars is defined in both EnergyFluxMod and VegetationDataType. When I switched to use veg_ef I didn't remove the energyflux_vars. I can fix this. But, does anyone know why many of the energyflux_vars are allocated in two subroutines?

removed the energyflux_vars

[Non-BFB] - Non Bit-For-Bit (crop model only)
@jqyin
Copy link
Contributor

jqyin commented Jul 12, 2019

@thorntonpe @qzhu-lbl can you approve this PR?

jqyin added a commit that referenced this pull request Jul 15, 2019
Adds climate driven planting date for crops

Introduces a new method to calculate crop planting dates. Calculates the
temperature or precipitation seasonality of a grid and determines planting
date from temperature or precipitation thresholds. Therefore, the growing
season is either the warm or wet season. This allows crops to grow globally,
rather than in just the temperate latitudes.

A new module, CropMod is added to support some of the calculations and
will serve as a home of future crop model developments that are specific
to crops. The parameter file is not changed as there are no new parameters.

Also, fixes the dynamic landunit bug that won't allow crops to be planted
if they weren't on the grid cell previously.

Fixes #2962
[Non-BFB] - Non Bit-For-Bit (only for the crop model)
@jqyin
Copy link
Contributor

jqyin commented Jul 15, 2019

Merged to next.

@rljacob
Copy link
Member

rljacob commented Jul 16, 2019

There were no diffs in the only crop test: SMS_Ly2_P1x1.1x1_smallvilleIA.ICLM45CNCROP.sandiatoss3_intel.clm-force_netcdf_pio Is that expected?

@bbye
Copy link
Contributor Author

bbye commented Jul 16, 2019

Yes, that is expected. The changes are minor for the temperature threshold, which is the case for the crop test. In other grid cells there will be changes. In a future PR I will update the parameter file with new values for temperature thresholds that will result in diffs, but I can't do that until #2992 and #2995 are fixed.

@jqyin jqyin merged commit 5c789dc into master Jul 16, 2019
jqyin added a commit that referenced this pull request Jul 16, 2019
Adds climate driven planting date for crops

Introduces a new method to calculate crop planting dates. Calculates the
temperature or precipitation seasonality of a grid and determines planting
date from temperature or precipitation thresholds. Therefore, the growing
season is either the warm or wet season. This allows crops to grow globally,
rather than in just the temperate latitudes.

A new module, CropMod is added to support some of the calculations and
will serve as a home of future crop model developments that are specific
to crops. The parameter file is not changed as there are no new parameters.

Also, fixes the dynamic landunit bug that won't allow crops to be planted
if they weren't on the grid cell previously.

Fixes #2962
[Non-BFB] - Non Bit-For-Bit (only for the crop model)
@jqyin
Copy link
Contributor

jqyin commented Jul 16, 2019

Merged #2999

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix PR Land non-BFB PR makes roundoff changes to answers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic Landunits won't work when a new crop is introduced
7 participants