-
Notifications
You must be signed in to change notification settings - Fork 92
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
base: main
Are you sure you want to change the base?
Conversation
de09c8e
to
c7af3ef
Compare
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:
|
…ction final ci value
…revert case to smooth c3 AcAj
… a trivial conductance solve when gs2=0
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not reviewing this earlier! I think the refactoring looks great and helps a lot to understand the photosynthesis code, thanks!
I just have some minor questions and suggestions.
…ling method to work with BB for type 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the work you did to continue modularizing the code. I have a couple of questions and identified a couple of commented out pieces of code.
<!-- --> | ||
|
||
<base_file>archive/base.cdl</base_file> | ||
<new_file>fates_params_default.cdl</new_file> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this will be updated closer to integration when we'll have a better idea what the default parameter file looks like at that time.
name = 'fates_daylength_factor_switch' | ||
call fates_params%RegisterParameter(name=name, dimension_shape=dimension_shape_scalar, & | ||
dimension_names=dim_names_scalar) | ||
|
||
name = 'fates_leaf_stomatal_model' | ||
call fates_params%RegisterParameter(name=name, dimension_shape=dimension_shape_scalar, & | ||
dimension_names=dim_names_scalar) | ||
|
||
name = 'fates_leaf_stomatal_assim_model' | ||
call fates_params%RegisterParameter(name=name, dimension_shape=dimension_shape_scalar, & | ||
dimension_names=dim_names_scalar) | ||
|
||
name = 'fates_leaf_photo_tempsens_model' | ||
call fates_params%RegisterParameter(name=name, dimension_shape=dimension_shape_scalar, & | ||
dimension_names=dim_names_scalar) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like having the parameter registration broken out into "domain" specific modules. I'm just flagging here that there are being migrated to the host land model in #1300. Given this and that there are new parameters, I'm now thinking we should bring this in prior to #1300 as it might be easier to deconflict in that order and would allow us to get the new parameters into the update associated with ESCOMP/CTSM#2904.
! subroutine LeafBiophysCheckParams() | ||
!write (fates_log(),*)'error, incorrect stomatal slope scaling method (stomatal_intercept_method) applied' | ||
!write (fates_log(),*)'valid options are:' | ||
!write (fates_log(),*)'btran_on_gs = 0, | ||
!write (fates_log(),*)'btran_on_gs = 1, apply btran to the stomatal conductance intercept' | ||
!write (fates_log(),*)'btran_on_gs_gs1 | ||
!write (fates_log(),*)'you specified stomatal_intercept_method = ',stomatal_intercept_method | ||
!call endrun(msg=errMsg(sourcefile, __LINE__)) | ||
! end if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave this in for future work?
call QuadraticRoots(aquad, bquad, cquad, r1, r2,err) | ||
agross = min(r1,r2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is checking err
output here not necessary?
call QuadraticRoots(aquad, bquad, cquad, r1, r2, err) | ||
|
||
ci = max(r1,r2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is checking err
here not necessary as well?
!if(maxval(psn_z(:,1,1))>nearzero)then | ||
! currentCohort => currentPatch%tallest | ||
! do while (associated(currentCohort)) ! Cohort loop | ||
! print*,currentCohort%gpp_tstep,currentCohort%pft,currentCohort%canopy_layer | ||
! currentCohort => currentCohort%shorter | ||
! enddo | ||
! stop | ||
!end if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code for future work, or temporary code to be removed?
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
Addresses: #719 (which is now a discussion)
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
Integrator
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: