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

leaf biophysics refactor and functional unit tests #1262

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

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented Oct 14, 2024

Description:

Leaf biophysics (photosynthesis, stomatal conductance and leaf respiration) has been refactored. Major objectives in this refactor were to 1) extract relevant routines that occur at the leaf-scale and place them in their own module that does not use the fates scaling data structures (cohort, patch and site agnostic) and 2) to have functional unit tests that exercise each of these functions.

These methods do not yet use features in our other brand of unit tests (but could!), particularly the use of cmake and parameter reading.

Design Notes

Notes on Bisection and finding bounding (starter) values

Notes on re-deriving the Medlyn solve with a boundary layer

Colab Notebook that runs the unit test: link

Fixes: #1222

Collaborators:

@adrifoster @ckoven

Expectation of Answer Changes:

There may be subtle differences in our standard integration tests. I do not expect the same results within round-off, but the diffs should be looked through to see if anything is very large.

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:

TBD

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

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

FATES baseline hash-tag:

Test Output:

@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 Oct 14, 2024
@rgknox
Copy link
Contributor Author

rgknox commented Nov 4, 2024

Full model regression testing is now showing nigh-indistinguishable results with base:

Test description: BCI, inventory initialization, ST3, 5 years, 2-stream rad, 1 tropical evergreen PFT, parameter defaults otherwise:

https://drive.google.com/file/d/1HJadTpwo_MAdL5Sr8qWK4EFV98WPTlXy/view?usp=sharing

Removing draft status

Next steps:

  1. evaluate gridded comparison while still retaining code that ensures near-b4b?
  2. incrementally remove b4b preservation code and evaluate differences
  3. clean up comments
  4. evaluate timing differences (particularly related to ci precision)
  5. introduce updates to btran (ie remove from stomatal intercept and add to Jmax)?

@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 Nov 4, 2024
@rgknox
Copy link
Contributor Author

rgknox commented Nov 4, 2024

I wanted to drop a note about expected changes with this refactor. There are a few places where results are changed with intent:

  1. we previously would abort the solution for Ci (intracellular CO2) if we encountered a negative Anet. This presumably was because negative Anet generates trivial conductance (intercept) which is a trivial solution. However, if the solver is not given a chance to converge properly, the next updates to Ci could very well generate positive Anet. In the refactor, we remove the premature abort on solution
  2. In the base model, we approach trivial solutions to cases with no leaves and no light kind of backwards. We see if the plant has light and then if the plant has leaves
  3. the convergence tolerance on Ci in master (EDIT...corrected) is pretty small, but finishes its search after 5 iterations and accepts the answer regardless of convergence.

I think for many cases, the old solution that we have in main actually does get us a reasonable solution, but it breaks down and gives poor results in extreme conditions (particularly very dry conditions). This is probably due to a combination of points 1 and 3, and that we didn't offer a highly-stable alternative to the simple recursive numerical search of Ci (we now use bisection if a solution has not been achieved in 10 tries)


I show two plots. Each plot looks at conductance, assimilation and Ci over a range of temperatures at BCI, for 4 combinations of absorbed PAR (high and low) and humidity (high and low). They both assume a boundary layer conductance of 2.5 mol/m2/s, which is on the high side for is experienced at BCI. They both use Medlyn conductance, and default parameters.

*Note the turquoise line in the Ci plots is the compensation point.

Using original code and Ci tolerance:

Photo_Stress_test_v7_high_cit_tol_med

Using new code and updated Ci tolerance:

Photo_Stress_test_v7_norm_cit_tol_nob4b_med


Here is the same comparison using Ball-Berry:

Original code:

Photo_Stress_test_v7_high_cit_tol

New Code:

Photo_Stress_test_v7_norm_cit_tol_nob4b

@glemieux glemieux self-requested a review November 18, 2024 17:38
@glemieux glemieux added refactor Restructures code without changing functionality testing Related to improvements in code testing (e.g. unit, regression, etc) labels Nov 18, 2024
@glemieux glemieux requested a review from mpaiao November 18, 2024 20:24
@rgknox
Copy link
Contributor Author

rgknox commented Nov 21, 2024

I added a new test script that is a pure smoke test (ie pass/fail) on getting a solution for as many combinations as possible. The test output is as follows:

Starting smoke tests with the following nested combinations:

 2 conductance models, Medlyn and Ball-Berry
 2 pathway (C3/C4) models
 6 stomatal btran options from 0 to 5
 3 agross btran options from 0 to 2
 15 leaf temperature values [C] from -50.0 to 60.0
 15 RH values [fraction] from 0.001 to 1.0
 15 PAR Abs [W/m2] values from 0.0 to 800
 10 BL conductance (gb) [umol/m2/s] values from 50000.0 to 8500000.0
 15 BTRAN values [fraction] from 0.006737946999085467 to 1.0
 10 Vcmax 25 top values [umol/m2/s] from 1 to 250

Running a total of 303750000 tests: 

Completed 15187500 tests -- 5.0 percent complete
Completed 30375000 tests -- 10.0 percent complete
Completed 45562500 tests -- 15.0 percent complete
Completed 60750000 tests -- 20.0 percent complete
Completed 75937500 tests -- 25.0 percent complete
Completed 91125000 tests -- 30.0 percent complete
Completed 106312500 tests -- 35.0 percent complete
Completed 121500000 tests -- 40.0 percent complete
Completed 136687500 tests -- 45.0 percent complete
Completed 151875000 tests -- 50.0 percent complete
Completed 167062500 tests -- 55.0 percent complete
Completed 182250000 tests -- 60.0 percent complete
Completed 197437500 tests -- 65.0 percent complete
Completed 212625000 tests -- 70.0 percent complete
Completed 227812500 tests -- 75.0 percent complete
Completed 243000000 tests -- 80.0 percent complete
Completed 258187500 tests -- 85.0 percent complete
Completed 273375000 tests -- 90.0 percent complete
Completed 288562500 tests -- 95.0 percent complete
Completed 303750000 tests -- 100.0 percent complete

Completed Photosynthesis Smoke Test


Elapsed Time [s]: 1121.01798985

0 Failures out of 303750000 Encountered; 0.0% of Tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Restructures code without changing functionality testing Related to improvements in code testing (e.g. unit, regression, etc)
Projects
Status: Under Review
Development

Successfully merging this pull request may close these issues.

leaf photosynthesis does not check for leaf area/stems and day/night correctly
2 participants