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

Adding day length factor switch #1161

Merged
merged 7 commits into from
Apr 26, 2024

Conversation

jenniferholm
Copy link
Contributor

@jenniferholm jenniferholm commented Feb 14, 2024

Adding in a user switch in the parameter file that turns day length factor on (1) by default, or off (2)

Description:

This pull request is adding in a user specified switch to turn on or off the day length scaling factor, that is applied to the photosynthetic parameters Vcmax25 and jmax25. This is to be able to allow backwards compatibility with previous versions of the calibration branch.
The function daylength is a scaling factor that is based on the formulation in Bauerle et al. (2012), which quantifies the relationship between day length and Jmax.

Collaborators:

Greg Lemieux

Expectation of Answer Changes:

The default is to have the day length factor turned on (equal to 1 in the parameter file), and should have the same results as the most recent calibration branch found here (https://github.com/rgknox/fates/tree/unsupported_sp_calibration_api31)

If turned off, this will change values related to photosynthesis, GPP, and downstream results.

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:

E3SM test hash-tag: a7f4ecb9dd

E3SM baseline hash-tag: a7f4ecb9dd

FATES baseline hash-tag: 42d804b
Test Output:

@glemieux glemieux self-assigned this Feb 26, 2024
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.

Thanks for getting this in @jenniferholm. I have one clarifying question that may require a code update.

biogeophys/FatesPlantRespPhotosynthMod.F90 Outdated Show resolved Hide resolved
This reduces code duplication somewhat by updating a local
value of the daylength factor based on the switch value.  I also
changed the switch to be zero for off and one for on to utilize
the existing itrue/ifalse constants.
@glemieux glemieux added stuck Pull request is currently waiting on resolution of an issue that came up during review or testing parameter file Pertaining to changes to the FATES parameter file labels Mar 19, 2024
@glemieux glemieux removed the stuck Pull request is currently waiting on resolution of an issue that came up during review or testing label Mar 21, 2024
@glemieux
Copy link
Contributor

Regression testing on derecho is underway.

@glemieux
Copy link
Contributor

glemieux commented Mar 22, 2024

Regression testing with the daylength switch turned off (set to 0) against the baseline fates-sci.1.72.3_api.34.0.0-ctsm5.1.dev174-default_allom create using the new default allometry parameters from #1093, shows all expected tests pass b4b, which is as expected.

Results: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1161-fates-dlswitch_off-pr1093basecompare

Testing with the switch on is in progress.

@glemieux
Copy link
Contributor

glemieux commented Mar 22, 2024

Regression testing with the switch on results in DIFFs across all fates test mods as expected. The first timestep appears to be affecting photosynthesis and gpp outputs.

results: /glade/u/home/glemieux/scratch/ctsm-tests/tests_pr1161-fates-dlswitch_on-pr1093basecompare

This is ready to integrate after #1128

@jenniferholm
Copy link
Contributor Author

Are there checks left to do on this PR? Jing needs this update in for her simulations, as do I for some updated CNP runs I'm trying to start. Maybe others do to for the global calibration? Let me know if there is anything I can do to help review. Thanks!

@glemieux
Copy link
Contributor

@jenniferholm nope, this is all good aside from testing. It's in the queue and we should be able to integrate it by next week.

@rgknox
Copy link
Contributor

rgknox commented Apr 26, 2024

testing now , tests passed, I temporarily reverted to daylength switch to be "off" and results are b4b, I will generate a new base with the daylength factor on

@rgknox rgknox merged commit 6c85a1b into NGEET:main Apr 26, 2024
1 check was pending
@rosiealice
Copy link
Contributor

Thanks @rgknox :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parameter file Pertaining to changes to the FATES parameter file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants