-
Notifications
You must be signed in to change notification settings - Fork 340
ctsm5.3.072: Incorporating a new vertical movement scheme for soil nitrate #2992
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
Conversation
|
Thanks, Jinmu! I'll plan to add some testing about this, so this checkbox is for me:
|
|
Agreed, thanks for turning this around so, quickly Jinmu. It's very exciting to see this work come through. From your perspective are there additional code modifications that need to be considered, or is this PR ready for us to start reviewing? |
|
So I don't forget, I'm less sure how we may need to make parallel changes in the matrix code, so also going to point @slevis-lmwg to this PR. |
|
I think you can start reviewing this code right now since it has been merged with ctsm5.3.027. I manually left a flag in my code; it works well for me, but is it better to put it into the namelist?
|
|
Two primary tests have been finished. |
Update ctsm_pylib to 3.13.2 update to newest ctsm
|
Are there namelist changes ( I also don't think we have time to investigate or implement potential changes to matrixCN on this timeline. Maybe that's a deal breaker for merging the PR? |
|
In SE meeting today @slevis-lmwg noted that Suggested we add aux_clm test for this, check that when use_nvmovement is off we get b4b results. Also suggesting we make use_nvmovement = true incomparable with MatrixCN, until we're convinced that matrixCN is compatable with this code change. Generally SE team was supportive of bringing this into master with the assumption that it will be off by default in CLM6. @slevis-lmwg can you add use_nvmovement to the namelist defaults and add an aux_clm test for this. |
|
@wwieder I'm happy to do this, and I have added to my Sprint board. Also, here are the notes from this morning's meeting:
I think this along with Will's post a few lines up means:
|
Based on what we said this morning -- it will depend. We will only bring it in earlier than that, if we can verify that it doesn't change answers when turned off. And that we can show that turning it on is at least functional. Also, other cesm3_0 release priorities may trump doing work on this, so we may not be able to get the release out AND do this work. So that could delay it as well. Since, we think of milestones as "the latest" acceptable, that is the correct milestone. |
|
@jinmuluo do you think you'd be able to give us an update on your simulations and the changes we could expect from simulations when we turn on this feature? |
@wwieder I have summarized some changes. Should I submit my findings here or copy them to you in email? some updates on the changes of soil C-N fluxes are listed here |
wwieder
left a comment
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.
This is great, @jinmuluo thanks for your work here.
Also thanks to @slevis-lmwg for prepping this merge.
Process question: My biggest concern with the PR is that it doesn't come with documentation. Maybe that's OK, or are we requiring documentaion for PRs like this to come it?
Science question were there other answer changing modifications to the CENTURY nitrif-denitrif scheme that went into the work you presented that altered nitrification rates, @jinmuluo? Or would these come in with @mvalmartin (or your) changes related to soil N2O and NOx (e.g. #3341)?
I can help with the documentation this fall when i visit NCAR, For the science part, the CENTURY scheme remained the same; my modifications on nitrification rate and denitrification are not in this PR or in (#3341), I will open another PR to modify the CENTURY nitrif-denitrif scheme; testing is still in progress. |
Merge b4b-dev to master slevis resolved conflicts: doc/ChangeLog doc/ChangeSum
|
@samsrabin I re-requested a review since you had requested changes before accepting to approve. |
samsrabin
left a comment
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.
Looks mostly good, thanks! But there are still some variable names that could use improving. We're not getting charged per character here!
|
|
|
@samsrabin I addressed your suggestions and reran aux_clm, so I will re-request your review. Thank you again! |
samsrabin
left a comment
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.
Perfect, thanks!
Incorporating a new vertical movement scheme for soil nitrate Work by Jinmu Luo of Cornell University. use_nvmovement defaults to .false. and can be changed in user_nl_clm. use_nvmovement cannot be true while use_nitrif_denitrif is false. We test the flag as .true. in two new tests: ERP_D_Ld5.f10_f10_mg37.I1850Clm60BgcCrop.derecho_intel.clm-nvmovement--clm-matrixcnOn ERP_D.f10_f10_mg37.IHistClm60Bgc.derecho_intel.clm-decStart--clm-nvmovement PR ESCOMP#2992
Hi
This method is proposed to solve the known nitrate leaching issue in CLM. Default CLM lacks the quick aqueous movement of nitrate in soil space, which means most of the soil nitrate keeps a fixed vertical profile across all time. It would cause a low nitrate leaching problem, as identified by Feng et al., 2023 (https://doi.org/10.1038/s41467-023-38803-z), Nevison et al., 2022( https://doi.org/10.1002/eap.2528), and many others.
We have proposed a physical-based method to force soil nitrate travel with the soil water vertically. Unlike the default nitrate leaching scheme in the CLM, where nitrate doesn't move downward and nitrate leaching happens in different layers. This new method can force nitrate to move vertically, and leached nitrate is taken out at the soil column bottom. This new method has been successfully implemented in our branch and can be used as an alternative leaching evaluation scheme in CLM. We are still working on the manuscript.
Description of changes
A new file used to evaluate the nitrate vertical movement: SoilNitrogenMovementMod.F90
Several files are modified to input the variables SoilNitrogenMovementMod.F90 needs.
Nitrate leaching can now be evaluated by our method or the default method by specifying a flag in SoilNitrogenMovementMod.F90
Specific notes
Contributors other than yourself, if any:
@samsrabin @wwieder
CTSM Issues Fixed (include github issue #):
Unknown
Are answers expected to change (and if so in what way)?
Our method increases the nitrate leaching flux and alters the size of other nitrogen fluxes.
Any User Interface Changes (namelist or namelist defaults changes)?
Users can use the flag 'use_nvmovement' to switch from the default method or our method.
This flag hasn't been written into the namelist, but stays in the new file SoilNitrogenMovementMod.F90
Does this create a need to change or add documentation? Did you do so?
it will need to change the documentation, we are still preparing our manuscript.
Testing performed, if any:
One year results are analysis here, I only list the changes of three main components.
Leaching.pdf
SurfaceRunoff.pdf
denitrification.pdf