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

Facilitate crops in SP mode. #817

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rosiealice
Copy link
Contributor

Description:

To allow FATES-SP mode to generate crop areas, this PR updates the default PFT file to extend the map to HLM_PFTs 15 and 16. This means that for grid cells with crops, area is allocated. In SP mode, the HLM actually shifts the crops onto the natural vegetation land unit, hence the previous solution (running over crop land units as well as soil land units) does not in itself solve the problem.

Further, this PR also changes the order of operations logic related to the precision checking of the total patch area. Previously, this happened AFTER the call to init_cohorts, which appeared to result in cohorts that were larger than the patch area with an error the same size and that later trimmed off by the precision checking in EDInit (see changes). This PR makes the call to init_cohorts occur after the change in patch areas to account for precision-level errors in the inputs.

Adding the crop PFTs appeared to trigger this error. It is possible that this change will fix #782 which threw a similar error.

This PR addresses issue #760

Collaborators:

Expectation of Answer Changes:

yes, this will change answers in SP and NOCOMP modes.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the in-code documentation .AND. (the technical note .OR. the wiki) accordingly.
  • I have read the CONTRIBUTING document.
  • FATES PASS/FAIL regression tests were run
  • If answers were expected to change, evaluation was performed and provided

Test Results:

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

@rosiealice
Copy link
Contributor Author

rosiealice commented Dec 6, 2021

TLAI comparison with CLM5 BEFORE change
SP_2rad_TLAI_43 (4)

TLAI comparison with CLM5 AFTER change.

SP_2rad_TLAI_41 (4)

@rosiealice
Copy link
Contributor Author

As you can see, there is still something up with the high arctic and the sahara, but that's likely nothing to do with crops...

@billsacks
Copy link
Member

Thanks for your work on this @rosiealice ! I haven't gotten my head back into all of the discussions in #760 but can you clarify how much of #760 is done here?

Two specific questions I have are:

(1) Regarding this:

In SP mode, the HLM actually shifts the crops onto the natural vegetation land unit, hence the previous solution (running over crop land units as well as soil land units) does not in itself solve the problem.

My understanding is that, at least eventually, we want crops still handled on their own landunits when running with FATES. Is that right? My understanding is also that, for non-FATES SP, CTSM puts crops on their own landunit (though I can never remember this for sure).

(2) If this PR is supposed to fully address #760, can you please make some comments about how much testing and/or code reading you've done to verify the various things in #760 that were raised along the lines of "we might need to do X / we should look into whether X is needed".

@ekluzek
Copy link
Collaborator

ekluzek commented Dec 7, 2021

@billsacks to your question

My understanding is also that, for non-FATES SP, CTSM puts crops on their own landunit (though I can never remember this for sure).

The answer is yes. We have a namelist flag for this that we only allow FATES to set to .false. (create_crop_landunit). We actually want to remove the namelist option and hardwire it to true.

@glemieux
Copy link
Contributor

@rosiealice per our conversion at the fates software meeting a couple mondays ago, I tested this branch out using a single pft with an f45 grid run. I'm still seeing the same failure mode from #782, so I think we need a separate fix for this issue. Results can be found on cheyenne here:
/glade/u/home/glemieux/scratch/ctsm-cases/spmode-pr817-newpfile-1pft.fates-sci.1.53.0_api.21.0.0-ctsm5.1.dev071-Cbcb9735e8-F3df5fa19.intel

@glemieux glemieux added the PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. label May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SP mode: model crashed with only 1 PFT
4 participants