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

multi-element litter, mass checks and diagnostics #524

Merged
merged 136 commits into from
Aug 26, 2019

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented Apr 23, 2019

Description:

This changeset refactors the core FATES code to cycle multiple elements beyond carbon12. This mostly is represented in three pieces of memory, the litter, diagnostics used in writing history diagnostics and mass balance checking and accounting. These changes have significant impacts on how we process input and output boundary conditions to the model.

This changeset also adds rooting profile to the cohort structure (although I think it may be useful to have a converstation about this and how to reduce memory needs here).

Specifics:

FatesLitterMod.F90

global: element_list()

site_fluxdiags_type

site_massbal_type

This PR should be synchronized with CTSM PR: .

Collaborators:

@ckoven, Bill Riley, @jkshuman, @walkeranthonyp, Fates Modeling Team

Expectation of Answer Changes:

The results in this PR should give reasonably similar results to previous simulations. However, so much code has changed, that I do expect round-off errors to be large enough to prevent B4B comparisons from passing (however, they may).

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:

TBD

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

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

FATES baseline hash-tag:

Test Output:

rgknox and others added 30 commits October 30, 2018 11:41
…, element chemical naming convention, and better organ looping.
…ruitment, shifted things so that the PRT object initialization is not a cohort object argument.
@rgknox
Copy link
Contributor Author

rgknox commented Jul 26, 2019

@jkshuman , I think I addressed all your comments, and accepted your suggestions. Let me know if I missed anything. I went into the EDPhysiology code and double checked how the resource_management diagnostics are filled, and I think the refactors maintain what is in master.

@rgknox
Copy link
Contributor Author

rgknox commented Jul 30, 2019

I ran some comparison simulations with an updated base branch that is based off of tag 1.29. Things are looking good. In this example, both the test and base are started with inventory at BCI, simulate for 8 years, and have logging, fire, disturbance and recruitment all active. I've also done similar tests with prescribed physiology mode as well.

https://drive.google.com/file/d/1QIROC4-NDLlX4veNpMvsPYAXVk49Qhtj/view?usp=sharing

Will continue testing, next is restart tests (again), hydro and then a long smoke test with the F10 grid. If anyone wants to get reviews in on this, please do so, hopefully we can get this integrated very soon.

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've gone through about half of the code changes, mostly focused on the new module addition and the modules that had smaller sets of changes. Per our face-to-face conversation, I'll probably need another day to finish.

biogeochem/FatesLitterMod.F90 Outdated Show resolved Hide resolved
main/EDPftvarcon.F90 Outdated Show resolved Hide resolved
biogeophys/FatesPlantRespPhotosynthMod.F90 Show resolved Hide resolved
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.

Done with part 2 or 2 of the review. I really like the way the litter is modularized in this refactor. I have a bunch of questions, mostly for clarification, one potential typo, and one suggested change. Nice work!

biogeochem/EDCohortDynamicsMod.F90 Show resolved Hide resolved
biogeochem/EDCohortDynamicsMod.F90 Show resolved Hide resolved
biogeochem/EDLoggingMortalityMod.F90 Show resolved Hide resolved
biogeochem/EDPhysiologyMod.F90 Outdated Show resolved Hide resolved
biogeochem/EDPhysiologyMod.F90 Outdated Show resolved Hide resolved
biogeochem/EDPhysiologyMod.F90 Show resolved Hide resolved
biogeochem/EDCohortDynamicsMod.F90 Show resolved Hide resolved
main/FatesInterfaceMod.F90 Outdated Show resolved Hide resolved
biogeochem/EDPatchDynamicsMod.F90 Outdated Show resolved Hide resolved
@rgknox rgknox removed the PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. label Aug 23, 2019
@rgknox
Copy link
Contributor Author

rgknox commented Aug 23, 2019

f10 simulations are progressing, in year 80:

/glade/scratch/rgknox/fates-clm-tests/me-litt-v3-f10_f10_mg37/run

Standard regression tests (no baseline comparison) all expected PASS:
/glade/scratch/rgknox/clmed-tests/litter-acnp-v5-C4e50a445-Fc416e641.fates.cheyenne.intel


seed_mass = num_dead * store_m * EDPftvarcon_inst%allom_frbstor_repro(pft)

! SEED DISTRIBUTION IS BREAKING MASS CONSERVATION RIGHT NOW...
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still the case and if so do we need to keep track of it with an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I was not able to get tight mass conservation on this, I will make an issue of this once we pull.
Note that we do allow storage mass to convert to seed in certain grasses, it just doesn't disperse and mix at the site level.

@rgknox
Copy link
Contributor Author

rgknox commented Aug 23, 2019

re-running regression tests, after applying updates from @glemieux

@rgknox
Copy link
Contributor Author

rgknox commented Aug 26, 2019

All expected PASS on the re-submission:
/glade/scratch/rgknox/clmed-tests/litter-acnp-v6-C4e50a445-F63b95b51.fates.cheyenne.intel

@rgknox
Copy link
Contributor Author

rgknox commented Aug 26, 2019

110 year f10 simulation completed without issue: /glade/scratch/rgknox/fates-clm-tests/me-litt-v3-f10_f10_mg37

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

Successfully merging this pull request may close these issues.

6 participants