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

time integrated flux diagnostics #1217

Merged
merged 30 commits into from
Oct 18, 2024
Merged

time integrated flux diagnostics #1217

merged 30 commits into from
Oct 18, 2024

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented Jul 1, 2024

Description:

This adds a site-level diagnostic that compares the total amount of mass in live vegetation (plants plus seeds) to the total integrated fluxes in and out of that pool over the life of the simulation. The same is true for the litter pool. History output for the bias on this comparison is added as well, to see if the bias grows over time.

Collaborators:

@JessicaNeedham

Expectation of Answer Changes:

This does have a bug fix, so some diagnostics related to litter fluxes may be corrected.

Checklist

If this is your first time contributing, please read the CONTRIBUTING document.

All checklist items must be checked to enable merging this pull request:

Contributor

  • The in-code documentation has been updated with descriptive comments
  • The documentation has been assessed to determine if updates are necessary

Integrator

  • FATES PASS/FAIL regression tests were run
  • Evaluation of test results for answer changes was performed and results provided

Documentation

Test Results:

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

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

FATES baseline hash-tag:

Test Output:

@rgknox rgknox changed the title integrated flux diagnostics time integrated flux diagnostics Jul 1, 2024
@rgknox rgknox added the PR status: Not Ready The author is signaling that this PR is a work in progress and not ready for integration. label Jul 1, 2024
@rgknox
Copy link
Contributor Author

rgknox commented Jul 3, 2024

The bias between integrated flux and live-vegetation and litter states are looking pretty good now, see plots below, the scale on liveveg and litter is e-8 and e-14 respectively.

I had to move the calculation of the flux integration to later in the call sequence so that termination mortality that occurs after vegetation dynamics is encapsulated. I'll also run a test with CNP to see if this was the cause of #1218 .

vegc
interr_litter
interr_liveveg

@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 12, 2024
@mpaiao mpaiao self-requested a review August 12, 2024 19:37
Copy link
Contributor

@mpaiao mpaiao left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the mass conservation assessment, @rgknox ! I went through the code and it looks great, I just had a couple of minor clarification questions and suggestions.

biogeochem/EDPhysiologyMod.F90 Outdated Show resolved Hide resolved
main/ChecksBalancesMod.F90 Show resolved Hide resolved
Copy link
Contributor

@JessicaNeedham JessicaNeedham left a comment

Choose a reason for hiding this comment

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

Thanks Ryan this looks great! My simulations with this branch merged also show very tiny divergence between integrated fluxes and stocks.

main/FatesHistoryInterfaceMod.F90 Outdated Show resolved Hide resolved
main/ChecksBalancesMod.F90 Outdated Show resolved Hide resolved

!print*, ediag%err_liveveg!, ediag%err_liveveg/ibal%state_liveveg, ibal%state_liveveg, net_uptake, tot_litter_input

! Perform the comparison between integrated flux and state
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we discussed this but not sure where we landed on having this turned on or not. Could it be an option so a user could opt in to having their run crash if states and integrated fluxes start to diverge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an issue here: #1264

@rgknox
Copy link
Contributor Author

rgknox commented Oct 17, 2024

@mpaiao and @JessicaNeedham I addressed your comments. @mpaiao I changed the name of that variable from leaf_* to surf_fine_* to be more general and include seed. @JessicaNeedham I added back in those history variables and also created an issue about graceful failure.

@rgknox
Copy link
Contributor Author

rgknox commented Oct 18, 2024

All tests b4b except for the specific output variables that were changed in this PR with intent.
/glade/derecho/scratch/rgknox/tests_1017-183514de

@rgknox rgknox merged commit f5bd625 into NGEET:main Oct 18, 2024
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants