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

Some logic fixes to nocomp and nocomp + fixed_biogeog #805

Merged
merged 4 commits into from
Nov 17, 2021

Conversation

ckoven
Copy link
Contributor

@ckoven ckoven commented Nov 5, 2021

This fixes things so that nocomp + fixed_biogeog works properly (it was crashing before during init) and also a fix for nocomp (it was allowing recruits on supposedly-disallowed patches).

I think this is why nocomp was super-slow before, because it was actually allowing and PFT to recruit on any patch. Now it doesn't.

In my head, I had previously thought that nocomp also implied fixed_biogeog. I realize now that it doesn't necessarily, and that the logic differs slightly between nocomp versus nocomp + fixed_biogeog. Anyway, all the various modes should work as intended now:

  • fixed_biogeog: only lets a given PFT exist on a gridcell if the map says it can
  • fixed_biogeog + nocomp: only lets a given PFT exist on a gridcell if the map says it can, and separates the gridcell into patch areas that each PFT can solely exist on based on the map areas
  • nocomp: splits up the map to give every possible PFT the same amount of space on every gridcell in which that PFT can solely exist on.

I think, in general, fixed_biogeog + nocomp is what people will want to use, not nocomp on its own, so we should probably add a test for a fixed_biogeog + nocomp configuration.

Description:

Collaborators:

Expectation of Answer Changes:

This should be answer-changing in nocomp, and allow the model to pass a nocomp + fixed_biogeog test (it wouldn't have before). All other modes should be identical.

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:

Copy link
Contributor

@glemieux glemieux left a comment

Choose a reason for hiding this comment

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

I suggested one minor refactor for simplicity, but this looks good as is.

main/EDInitMod.F90 Outdated Show resolved Hide resolved
if(currentSite%use_this_pft(ft).eq.itrue &
.and. ((hlm_use_nocomp .eq. ifalse) .or. (ft .eq. currentPatch%nocomp_pft_label)))then

temp_cohort%canopy_trim = init_recruit_trim
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the crux of this is that the use_this_pft flag is a site level and not patch level quantity. I wonder
a) whether use_this_pft should be a patch level quantity.
b) if not, could we put a comment where it is calculated (or here) to explain why this patch-level logic is required in nocomp mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rosiealice I added the comment you requested above.

@glemieux glemieux self-assigned this Nov 15, 2021
@glemieux
Copy link
Contributor

glemieux commented Nov 15, 2021

All expected tests pass b4b, with the exception of a baseline DIFF for the one no comp testmod, which is also as expected. Test results are here:
/glade/u/home/glemieux/scratch/ctsm-tests/tests_pr805-nocompfixes

@glemieux
Copy link
Contributor

@ckoven am I remembering correctly that you had suggested we add a regression test covering the use of both nocomp + fixed biogeography modes? If so, I'll add an issue on the ctsm-side and roll that into one of the ctsm-side cleanup PRs I have going.

@ckoven
Copy link
Contributor Author

ckoven commented Nov 15, 2021

@glemieux yes please do add a nocomp+fixed_biogeog test into one of the CTSM PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants