Merging soil NO into the new vertical movement scheme for soil nitrate#3341
Merging soil NO into the new vertical movement scheme for soil nitrate#3341mvalmartin wants to merge 7 commits intoESCOMP:masterfrom
Conversation
slevis-lmwg
added
the
enhancement
|
This is based off of: ctsm5.3.040 There is new work to update this to a newer version of CTSM. |
ekluzek
added
the
next
|
Adding next to make sure we know what is happening with all this work. |
ekluzek
removed
the
next
|
@wwieder and @slevis-lmwg will review. @slevis-lmwg meets with the group Monday, and will get more information then. |
There was a problem hiding this comment.
@mvalmartin for starters I want to say that your coding here seems to me well organized, so thank you for this.
In my review I think I only made one comment, but you will find that I repeated it a few times.
Another request is this: Could you document in the PR (if you didn't already) how you tested your code changes? What types of simulations you performed (compsets, lengths, resolutions, ...)? What history fields you looked at to confirm model behavior? What model behavior you expected and what model behavior you saw?
src/biogeochem/DryDepVelocity.F90
Outdated
| if (clmveg == nc3crop .or. clmveg == nc3irrig ) ratio_stai_lai = 0.008_r8 | ||
| if (clmveg >= npcropmin .and. clmveg <= npcropmax ) ratio_stai_lai = 0.008_r8 | ||
|
|
||
| crf_drydep(pi)=(exp(-11.6_r8*elai(pi)*ratio_stai_lai)+exp(-0.32_r8*elai(pi)))/2 |
There was a problem hiding this comment.
This seems to be one big new section of code.
A comment for here and elsewhere:
When possible, we avoid hardwiring values without explanation. As a first step, could you name things like -11.6 and -0.32 to variable names?
There was a problem hiding this comment.
I also wonder if values like ratio_stai_lai should be PFT parameters on the parameter file to make this code easier to read? The only odd part is that values change by season for some PFTs?
There was a problem hiding this comment.
Thanks for the note and apologies for the slow 'action'. Working on your comments now!
There was a problem hiding this comment.
Thanks for the note and apologies for the slow 'action'. Working on your comments now!
@mvalmartin a heads up that I'm about to update your branch to the latest escomp/master tag. I'm mentioning in case you already started making changes to your local copy. If so, you would
- "stash" your local changes (git stash)
- pull my remote changes (git pull)
- bring forward your local changes (git stash pop)
- continue making changes as needed (git add, git commit, git push)
I hope all that works without trouble...
There was a problem hiding this comment.
If you didn't already start making changes, then you can simply skip the "git stash" and "git stash pop" steps. In other words just
- git pull
- make changes
- git add
- git commit
- git push
| if ( h2osoi_diff(c,j) > 0.005_r8 ) then | ||
| ! Initialize new pulse factor (dry period hours) | ||
| if ( ldry_vr(c,j) > 72._r8 ) then | ||
| pfactor_vr(c,j) = 13._r8 *LOG(ldry_vr(c,j)) - 53.6_r8 |
There was a problem hiding this comment.
This is another large section of the new code. It would be helpful again here to assign hardwired values to variable names (always with literature references when possible).
There was a problem hiding this comment.
Thanks for this comment. In the original code the hardwired values appear as simple scalars in the equations, without variable names or descriptions. I have now added the reference (article and equation number) for each equation to make this clearer.
| afps_vr(c,j) = 1._r8-max(min(h2osoi_vol(c,j)/watsat(c,j), 1._r8), 0._r8) | ||
| Dr = 0.209_r8*afps_vr(c,j)**(4._r8/3._r8) | ||
|
|
||
| nox_n2o_ratio_vr(c,j) = 15.2_r8 + (35.5_r8 * atan(0.68_r8 * rpi * (10.0_r8 * Dr - 1.86_r8))) / rpi |
There was a problem hiding this comment.
This is another area with numerous hardwired values that need some kind of documentation, such as assigning variable names and literature refs to them.
There was a problem hiding this comment.
In this case too. In the original code the hardwired values appear as simple scalars in the equations, without variable names or descriptions. I added the reference (article and equation number) for each equation to make this clearer.
src/biogeochem/DryDepVelocity.F90
Outdated
| if (clmveg == nc3crop .or. clmveg == nc3irrig ) ratio_stai_lai = 0.008_r8 | ||
| if (clmveg >= npcropmin .and. clmveg <= npcropmax ) ratio_stai_lai = 0.008_r8 | ||
|
|
||
| crf_drydep(pi)=(exp(-11.6_r8*elai(pi)*ratio_stai_lai)+exp(-0.32_r8*elai(pi)))/2 |
There was a problem hiding this comment.
I also wonder if values like ratio_stai_lai should be PFT parameters on the parameter file to make this code easier to read? The only odd part is that values change by season for some PFTs?
|
@mvalmartin I will need "collaborator access" to your branch for when I end up working on it. This is how you give me access:
|
|
@slevis-lmwg, I added you as collaborator in the PR, you should have received an invitation. Please let me know if you can't still access it. Thanks! |
…_soilno slevis resolved conflicts: src/main/controlMod.F90
|
@mvalmartin thank you. I confirmed my access by merging the latest updates from @jinmuluo's branch to your branch here. A few points:
|
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 Sam, The problem is specifically with the "meg_coef" parameter that didn't exist in the earlier version. I'm not 100% sure what this parameter does, but when we remove it and leave that section of the code as in ctsm5.3.040, the test passes successful. For reference, this is the change we made: Thanks a lot! Maria |
Hi Maria, Just my personal guess, did you update the submodule in your code repository? like the CMEP (your_ctsmc_ode/components/cmeps), please check the newest updates for the MEGAN emission coefficient in shr_megan_mod.F90 (commits below), you should find that the "coeff" has been moved to a new type in the newest ctsm. |
|
Yes, MEGAN needs to be coordinated with the components/cmeps submodule. So run the following just to check if something needs to be updated (without actually changing anything): ./bin/git-fleximod status It'll report if the submodules are all correct or not. If not you can run the above with "update" rather than "status" to get it in sync. |
Hello @jinmuluo and @ekluzek, Thanks a lot for your feedback. That was the issue. |
|
@mvalmartin and Ru, just to be clear, by "the test is successful" do you mean the same test that was failing for me before in the RUN phase? If so, please push your updates to this PR, so that I may rerun the full aux_clm. |
Yes, your test ~cime/script/create_test SMS.f10_f10_mg37.I2000Clm50BgcCrop.derecho_intel.clm-crop I have just pushed the updates. I hope this helps! |
Thank you, Maria. The last commit that I see here in the PR is your commit from Sep 1st. Could you try pushing again. I wonder if you committed but did not push. |
Hi Sam, Sorry for the confusion!! I think I mixed up “updating my local checkout for testing” with “pushing changes to the PR”. I did run some git commands (including commit/push), assuming the git-fleximod sync work would show up in the PR, but after checking I can confirm the PR branch hasn’t changed (both my local branch and the gitgub branch still point to the same commit (8fac120), and git diff is clean.) So any git-fleximod updates I did were just to get my local environment in sync for testing and did not end up as new PR commits. Ru is looking at starting the spin-up. Will recommended to follow this case NCAR/LMWG_dev#91 but we wanted to check with you as that was last summer and things may have changed since then. |
Perfect, Sam. For testing the scheme, is there a case we can clone or follow? Ru is looking at this link shared by @wwieder NCAR/LMWG_dev#91 |
|
@mvalmartin I don't know what we're doing differently, but the same test returned: The test gets submitted from here: |
Hi @slevis-lmwg, did you sync the components/cmeps submodule as pointed out by Erik? The only thing i did on my version was running ./bin/git-fleximod status |
|
This doesn't seem to be the problem. The status command returns this before and after I run the update: Cime and doc-builder almost always show these errors. When I go in /cime, git describe says cime6.1.113. |
|
Just in case something got corrupted with my version, I will start with a clean checkout and test again. |
|
Still fails for me... here, same traceback: |
|
One difference between Maria's case and Sam's case would be the 'use_soil_nox' activated or not? Sam's case broken at the point when CLM tried to map the patch level canopy reduction factor to column level in SoilBiogeochemCompetitionMod.F90 which is associated with Maria's nox emission, and in Sam's case, use_soil_nox is false. |
Thanks a lot, @jinmuluo! My version also runs with use_soil_nox=false although I see I left dry deposition active as it is needed for soil nox emissions. |
|
GOOD NEWS: I added -drydep to the same test and executed the following commands: and it passed. You're correct @mvalmartin that we need to have all this working for the default version, as well. |
|
Follow-up thought: I did not think of it this way earlier. Is it that we need to make -drydep the default? |
I guess we do? I don't know how to code that though. If it is not necessary for other processes in clm5 (i guess that was the reason it went to off by default) , users just need to be aware that use_soil_nox=true needs to have 'drydep' on.? Thanks! |
|
In an I case I don't think we need Drydep on, do we? |
drydep needs to be on in the I cases for use_soil_nox=true because the canopy reduction factor is calculated there but it doesnt need to be on for use_soil_nox=false |
@mvalmartin you expect I added Same failure as before. |
This was @jinmuluo's assessment of the bug. |
… on with use_soil_nox=false
Hi, apologies for the delay. I realized that the canopy reduction factor was outside the use_soil_nox constraint and therefore required dry deposition to be turned on. I have now added an IF statement so that dry deposition is no longer required. (When I checked my test script, I noticed that I had left dry deposition turned on, which is why it appeared to work before.) I have committed the updated F90 module to the PR. Please let me know if anything else is needed from my end. I would also like to run a spin-up to show that the soil NOx addition does not affect the carbon cycle. Could someone point me to a case I could clone for this comparison? Thanks! |
5 participants
Description of changes
This PR merges the soil NO from #2290 into the NO3 vertical movement scheme in #2992.
The soil NO scheme is based on Parton et al (2001), and includes canopy reduction (Yan et al, 2005) and rain pulses (Yan et al 2005; Hudman et al 2012).
The scheme uses a fixed soil pH value of 6.5, as defined in SoilBiogeochemCompetitionMod.F90
In addition, the scheme does not includes the dependency of the N mineralization-based term on potential nitrification rates proposed in Parton et al. (2001), which is missing in CLM5 as pointed out by Nevison et al., (2022). Although that was implemented in #2290, it is now removed because it substantially affected N plant uptake and the broader carbon cycle.
A more complex version of this scheme, which includes spatially distributed soil pH, weathering effects on denitrification and soil NH3 volatilization, is described and validated in Val Martin et al (2023).
References
Hudman, R. C., Moore, N. E., Mebust, A. K., Martin, R. V., Russell, A. R., Valin, L. C., and Cohen, R. C.: Steps towards a mechanistic model of global soil nitric oxide emissions: implementation and space based-constraints, Atmos. Chem. Phys., 12, 7779–7795, https://doi.org/10.5194/acp-12-7779-2012, 2012.
Nevison, C., Goodale, C., Hess, P., Wieder, W. R., Vira, J., and Groffman, P. M.: Nitrification and Denitrification in the Community Land Model Compared with Observations at Hubbard Brook Forest, Ecol. Appl., 32, e2530, https://doi.org/10.1002/eap.2530, 2022
Parton, W. J., Holland, E. A., Del Grosso, S. J., Hartman, M. D., Martin, R. E., Mosier, A. R., Ojima, D. S., and Schimel, D. S.: Generalized model for NOx and N2O emissions from soils, J. Geophys. Res.-Atmos., 106, 17403–17419, https://doi.org/10.1029/2001JD900101, 2001.
Val Martin, M., Blanc-Betes, E., Fung, K. M., Kantzas, E. P., Kantola, I. B., Chiaravalloti, I., Taylor, L. L., Emmons, L. K., Wieder, W. R., Planavsky, N. J., Masters, M. D., DeLucia, E. H., Tai, A. P. K., and Beerling, D. J.: Improving nitrogen cycling in a land surface model (CLM5) to quantify soil N2O, NO, and NH3 emissions from enhanced rock weathering with croplands, Geosci. Model Dev., 16, 5783–5801, https://doi.org/10.5194/gmd-16-5783-2023, 2023.
Yan, X., Ohara, T., and Akimoto, H.: Statistical modelling of global soil NOx emissions, Global Biogeochem. Cy., 19, GB3019, https://doi.org/10.1029/2004GB002276, 2005.
Specific notes
Contributors other than yourself, if any: @lkemmons, @slevis-lmwg & @wwieder
CTSM Issues Fixed (include github issue #): Unknown
Are answers expected to change (and if so in what way)? I don't think so
Any User Interface Changes (namelist or namelist defaults changes)?
The soil NO scheme can be turned on and off within the user_nl_clm as use_soil_nox=.true. or .false.
As indicated by Jinmu Luo, the soil NO3 vertical movement scheme can also be turned on and off, but the switch is hardwire within SoilNitrogenMovementMod.F90
Does this create a need to change or add documentation? Did you do so?
To be determined
Testing performed, if any:
We performed 15-year runs using the new configuration, with the first 5 year treated as a pseudo-spin up, using the compset 2000_DATM%GSWP3v1_CLM50%BGC-CROP_SICE_SOCN_MOSART_CISM2%NOEVOLVE_SWAV
For evaluation we analyzed global annual timeseries and spatial distributions including changes between runs. Simulations were:
The main changes in the N cycle (eg soil NO2, denitrification and nitrfication) are driven by the NO3 vertical movement implementation. No significant changes are noticed in the carbon cycle (eg NPP, TOTECOSYSC).
plot_no3vertmov_soilnox_mvalmartin.pdf