Skip to content

Conversation

@mvalmartin
Copy link

This 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)

It is not connected to soil pH from the surface file, that is, it considers the fixed soil pH value of 6.5 given in
SoilBiogeochemCompetitionMod.F90

It also includes the dependency of the N mineralization-based term on potential nitrification rates that was implemented in Parton et al. (2001), which was missing
from previous versions of CLM5 as pointed out by Nevison et al., (2022).

This scheme -with more complexity as it is linked to spatially distributed soil pH and considers weathering effect on denitrification and NH3 volatilization- is described and evaluated 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.

Description of changes

Specific notes

Contributors other than yourself, if any:

CTSM Issues Fixed (include github issue #):

Are answers expected to change (and if so in what way)?

Any User Interface Changes (namelist or namelist defaults changes)?

Testing performed, if any:
(List what testing you did to show your changes worked as expected)
(This can be manual testing or running of the different test suites)
(Documentation on system testing is here: https://github.com/ESCOMP/ctsm/wiki/System-Testing-Guide)
(aux_clm on cheyenne for intel/gnu and izumi for intel/gnu/nag/pgi is the standard for tags on master)

NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).

This 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)

It is not connected to soil pH from the surface file, that is, it
considers the fixed soil pH value of 6.5 given in
SoilBiogeochemCompetitionMod.F90

It also includes the dependency of the N mineralization-based term on
potential nitrification rates that was implemented in Parton et al.
(2001), which was missing
from previous versions of CLM5 as pointed out by Nevison et al., (2022).

This scheme -with more complexity as it is linked to spatially
distributed soil pH and considers weathering effect on denitrification
and NH3 volatilization- is described and evaluated 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.
@ekluzek ekluzek added enhancement new capability or improved behavior of existing capability tag: enh - new science labels Dec 13, 2023
@wwieder
Copy link
Contributor

wwieder commented Dec 14, 2023

Thanks for making this PR, @mvalmartin ! We'll start reviewing the code from a scientific and software perspective to see how to move forward. This may be a bit slow with AGU and the holidays.

Can you remind me if you've tried spinning these case up from bare ground and creating new initial conditions? I'm curious how much we should expect these nitrogen-focused changes to influence carbon stocks / fluxes?

@mvalmartin
Copy link
Author

Hi, for the testing I used the spin-up file from Val Martin et al (2023). The changes in the N cycling are not exactly the same but I thought the spin-up file could be used as a good approximation to assess at the spatial distribution, magnitude and seasonality of soil NOx and compared them to other estimates. Perhaps a new spin-up would be necessary to check on the carbon stocks/pools?
Maria

@samsrabin samsrabin added next this should get some attention in the next week or two. Normally each Thursday SE meeting. and removed next this should get some attention in the next week or two. Normally each Thursday SE meeting. labels Jan 11, 2024
@samsrabin samsrabin self-assigned this Jan 11, 2024
@wwieder
Copy link
Contributor

wwieder commented Jan 11, 2024

Hi @mvalmartin,

I wanted to let you know that @samsrabin is going to review this PR from the software side. I'll look at it from the science side. Realistically I don't think this with be addressed until after the Feb WG meetings, but it sounds like the the CAMChem group wants this for CESM3 (e.g. before June 2024). Hopefully this timeline is OK with @tilmes?

Copy link
Contributor

@wwieder wwieder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include a namelist variable to define the method by which N gasses are being calculated (e.g. Koven2013 vs. Valmartin2023). This would facilitate backwards capabilities with CLM5 (vs. CLM6). It also makes the code a little harder to read / understand, but is likely a good way to test modifications to the code don't change answers?

smin_no3_leached => soilbiogeochem_nitrogenflux_inst%smin_no3_leached_col , & ! Input: [real(r8) (:) ] (gN/m2/s) soil mineral NO3 pool loss to leaching
smin_no3_runoff => soilbiogeochem_nitrogenflux_inst%smin_no3_runoff_col , & ! Input: [real(r8) (:) ] (gN/m2/s) soil mineral NO3 pool loss to runoff
f_n2o_nit => soilbiogeochem_nitrogenflux_inst%f_n2o_nit_col , & ! Input: [real(r8) (:) ] (gN/m2/s) flux of N2o from nitrification
!mvm soil NOx 06/19/2023
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mvalmartin here and throughout the PR can you remove the comments with your initials and the date

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and please also delete commented-out lines of code that your new code is replacing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that, no problem!

call hist_addfld1d ( fname='RS_DRYDEP_O3', units='s/m', &
avgflag='A', long_name='Stomatal Resistance Associated with Ozone Dry Deposition Velocity', &
ptr_patch=this%rs_drydep_patch, default='inactive' )
!mvm for soil NOx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this variable have a more descriptive name (e.g., CRF_DRYDEP_NO), similar to O3, above?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noted!

rc = 1._r8/((1._r8/rsmx(ispec))+(1._r8/rlux(ispec)) + &
(1._r8/(rdc+rclx(ispec)))+(1._r8/(rac(index_season,wesveg)+rgsx(ispec))))
rc = max( 10._r8, rc)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like setting the parameter ratio_sai_lai for each PFT may be better accomplished on the CLM parameter file, instead of this complicated logic?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, 2-d variable with dimensions for PFT and season would make it easier for changes and sensitivity tests in the future.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to do that, I'm not really familiar with the CTSM coding convention though so not sure how to create this 2-D variable with season and PFT.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't realize this was a time varying variable. Since this is something we're already calculating elsewhere in the model, should we just use the ratio of SAI and LAI for consistency? I realize this may change answers scientifically that you'd want to evaluate? The easier option would be to keep this as is for CLM6/CESM3?

Copy link
Author

@mvalmartin mvalmartin Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it as it is for now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The model has another sai = stem area index. Better to call this one stai to prevent confusion?


! Account for nitrification fluxes
ns%smin_nh4_vr_col(c,j) = ns%smin_nh4_vr_col(c,j) - nf%f_nit_vr_col(c,j) * dt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the unused code below. Alternatively, keep it if we're going to implement a namelist variable to determine how N gasses are being calculated?

f_nox_denit_atmos => soilbiogeochem_nitrogenflux_inst%f_nox_denit_atmos_col , & ! Input:[real(r8) (:) ] (gN/m2/s) flux of NOx from denitrification; added by mvm for NOx emission
f_nox_denit => soilbiogeochem_nitrogenflux_inst%f_nox_denit_col , & ! Input:[real(r8) (:) ] (gN/m2/s) flux of NOx from denitrification; added by mvm for NOx emission
f_nox_nit_atmos => soilbiogeochem_nitrogenflux_inst%f_nox_nit_atmos_col, & ! Input: [real(r8) (:) ] (gN/m2/s) flux of NOx from nitrification; added by mvm for NOx emission

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there be an f_nox_nit flux (similar to the f_nox_denit to parallel the flux that's lost to the atmosphere?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in theory it should be a f_nox_nit as well. I'll check the code!

Copy link
Author

@mvalmartin mvalmartin Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, only the _atmos fluxes are lost to the atmosphere. The f_nox_denit was there for debugging, in case there is an issue with the N balance and can be output during debugging. I'll add f_nox_nit for consistency.


! final ratio expression
n2_n2o_ratio_denit_vr(c,j) = max(0.16_r8*ratio_k1(c,j), ratio_k1(c,j)*exp(-0.8_r8 * ratio_no3_co2(c,j))) * fr_WFPS(c,j)
! n2_n2o_ratio_denit_vr(c,j) = max(0.16_r8*ratio_k1(c,j), ratio_k1(c,j)*exp(-0.8_r8 * ratio_no3_co2(c,j))) * fr_WFPS(c,j)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unused code or define what's being used with namelist variables

fno3_co2 = max(0.16_r8*ratio_k1(c,j), ratio_k1(c,j)*exp(-0.8_r8 * ratio_no3_co2(c,j)))
n2_n2o_ratio_denit_vr(c,j) = max(0.1_r8,fno3_co2*fr_WFPS(c,j))

!mvm 06/15/2023 NOx FLUXES
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are great comments, but they may seem better suited to the tech note?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree for the most part, but I think the comment at line 424 can stay. The other comments are redundant with comments at the variable definition lines; the one at 424 explains what the calculation is doing.

c = filter_soilc(fc)
this%denit_col(c) = this%f_denit_col(c)
!mvm 06/19/2023 for coupling. Total soil N fluxes pass to the atmosphere
this%soil_nox_total_col(c)=this%f_nox_nit_atmos_col(c)+this%f_nox_denit_atmos_col(c)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this flux be include _atmos somewhere in the name since it's actually the total soil NOx flux to the atmosphere?

crf_drydep_col(c)= min(1._r8, max(0._r8, crf_drydep_col(c)))

f_nox_nit_atmos_vr(c,j) =f_nox_nit_vr(c,j)* crf_drydep_col(c)
f_nox_denit_atmos_vr(c,j)=f_nox_denit_vr(c,j)*crf_drydep_col(c)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused where the NOx that's trapped in the canopy come back to the soil. Does this just occur because only the f_nox_nit_atmos_vr_col is actually removed from the smin_no3_vr_col pool in N_StateUpdate (below)?

ns%smin_no3_vr_col(c,j) = ns%smin_no3_vr_col(c,j) - nf%f_nox_nit_atmos_vr_col(c,j) * dt

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, only the NO that is not trapped in the canopy exits the system and is paased into the atmosphere. Both f_nox_nit_atmos and f_nox_denit_atmos leave the system. NO trapped in the canopy ideally should contribute to plant nitrogen uptake, but there are uncertainties in how plants process that nitrogen and how we can parameterize these processes.

sminn_to_plant_fun_no3_vr(c,j),smin_no3_to_plant_vr(c,j),f_denit_vr(c,j),f_nit_vr(c,j),f_n2o_denit_vr(c,j),f_n2o_nit_vr(c,j),f_nox_denit_vr(c,j),f_nox_nit_vr(c,j),n2_n2o_ratio_denit_vr(c,j),nox_n2o_ratio_vr(c,j),f_n2_denit_vr(c,j),crf_drydep_col(c),pfactor_vr(c,j),pot_f_nox_denit_vr(c,j),pot_f_nox_nit_vr(c,j),pot_f_denit_vr(c,j),pot_f_nit_vr(c,j),(smin_no3_vr(c,j) / dt),actual_immob_no3_vr(c,j)
call endrun("too much NO3 uptake predicted by FUN")
end if
!KO if ((sminn_to_plant_fun_nh4_vr(c,j)-smin_nh4_to_plant_vr(c,j)).gt.0.0000000000001_r8) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe remove this redundant line too.

Copy link
Member

@samsrabin samsrabin 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 this! I have only a few minor comments on top of what Will already mentioned.

I especially agree that it would be nice for testing purposes to make the new stuff optional, at least temporarily.

smin_no3_leached => soilbiogeochem_nitrogenflux_inst%smin_no3_leached_col , & ! Input: [real(r8) (:) ] (gN/m2/s) soil mineral NO3 pool loss to leaching
smin_no3_runoff => soilbiogeochem_nitrogenflux_inst%smin_no3_runoff_col , & ! Input: [real(r8) (:) ] (gN/m2/s) soil mineral NO3 pool loss to runoff
f_n2o_nit => soilbiogeochem_nitrogenflux_inst%f_n2o_nit_col , & ! Input: [real(r8) (:) ] (gN/m2/s) flux of N2o from nitrification
!mvm soil NOx 06/19/2023
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and please also delete commented-out lines of code that your new code is replacing.

rc = 1._r8/((1._r8/rsmx(ispec))+(1._r8/rlux(ispec)) + &
(1._r8/(rdc+rclx(ispec)))+(1._r8/(rac(index_season,wesveg)+rgsx(ispec))))
rc = max( 10._r8, rc)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, 2-d variable with dimensions for PFT and season would make it easier for changes and sensitivity tests in the future.

if (h2osoi_vol(c,j) < 0.30_r8 .and. pfactor_vr(c,j) .eq. 1._r8) then

! If change in soil moisture is > 0.005 (0.5% v/v) and there is precipitation, then start pulsing
if ( h2osoi_diff(c,j) > 0.005_r8 ) then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align this and subsequent comments with the lines following.

pH(bounds%begc:bounds%endc) = 6.5_r8 !!! set all soils with the same pH as placeholder here
co2diff_con(1) = 0.1325_r8
co2diff_con(2) = 0.0009_r8
!mvm 06/15/2023 added to correct for missing organic matter turn-over
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the parameter file, please!

fno3_co2 = max(0.16_r8*ratio_k1(c,j), ratio_k1(c,j)*exp(-0.8_r8 * ratio_no3_co2(c,j)))
n2_n2o_ratio_denit_vr(c,j) = max(0.1_r8,fno3_co2*fr_WFPS(c,j))

!mvm 06/15/2023 NOx FLUXES
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree for the most part, but I think the comment at line 424 can stay. The other comments are redundant with comments at the variable definition lines; the one at 424 explains what the calculation is doing.

@wwieder
Copy link
Contributor

wwieder commented Jan 12, 2024

@mvalmartin and @samsrabin, what's our best plan moving forward? Some of the requests are easy (remove your name and date from line changes). Others may be more challenging (adding parameters to parameters to the parameter file and using namelist flags)?

We're pretty swamped with SE needs for CESM3, so any work your group can contribute would be appreciated, Maria. That said, some of this may be simpler for someone who's more familiar with CTSM coding conventions? Happy to hear your thoughts on this.

@mvalmartin
Copy link
Author

I have to admit that some of my coding practices are rustic :) I can modify some of the code in the next days -before I start lecturing again. I'm not fully familiar with the CTSM coding conventions so may need some help, specially about adding the namelist or parameters in the file. I'll do my best though.

@samsrabin
Copy link
Member

samsrabin commented Jan 16, 2024

Definitely feel free to ping me if you need help with the parameter file! Or anything else for that matter. You can mention me here or send me an email.

@wwieder wwieder added the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Jan 18, 2024
@samsrabin samsrabin removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Jan 18, 2024
@wwieder
Copy link
Contributor

wwieder commented Jan 18, 2024

@samlevis will you look at how this may integrate with #640? I'm assuming Matrix will come in before this PR is ready.

@wwieder wwieder requested a review from slevis-lmwg January 18, 2024 17:26
@wwieder wwieder marked this pull request as draft January 18, 2024 17:26
@wwieder wwieder added this to the CESM3 milestone Jan 18, 2024
@wwieder
Copy link
Contributor

wwieder commented Feb 7, 2024

Hi @mvalmartin , One more question that came up in a conversation with @ekluzek and @tilmes was that we don't have the rest of the CESM PR here, but can you also open a PR on CMEPS for passing these fluxes through the coupler?

@mvalmartin
Copy link
Author

Thanks! I added it into my to do list!

Copy link
Contributor

@slevis-lmwg slevis-lmwg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mvalmartin thank you for this contribution! From my perspective, once

  • revisions have been made to address the code reviews,
  • the PR has been updated to the latest clm tag
  • test-suites have passed and
  • this PR can be merged to clm's main trunk (aka master),
  • I will need to figure out how to make this PR compatible with the cn-matrix_v3 branch (PR #640), which will be coming into main soon.

write(iulog,*)'net flux = ',(col_ninputs(c)-col_noutputs(c))*dt
write(iulog,*)'inputs,ffix,nfix,ndep = ',ffix_to_sminn(c)*dt,nfix_to_sminn(c)*dt,ndep_to_sminn(c)*dt
write(iulog,*)'outputs,lch,roff,dnit = ',smin_no3_leached(c)*dt, smin_no3_runoff(c)*dt,f_n2o_nit(c)*dt
write(iulog,*)'outputs,lch,roff,dnit = ',smin_no3_leached(c)*dt, smin_no3_runoff(c)*dt,denit(c)*dt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure that denit is calculated somewhere. I did not see it anywhere but may have missed it.

rc = 1._r8/((1._r8/rsmx(ispec))+(1._r8/rlux(ispec)) + &
(1._r8/(rdc+rclx(ispec)))+(1._r8/(rac(index_season,wesveg)+rgsx(ispec))))
rc = max( 10._r8, rc)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The model has another sai = stem area index. Better to call this one stai to prevent confusion?

@mvalmartin
Copy link
Author

Hi @slevis-lmwg, @wwieder and @samsrabin, Apologies I'm slow working on this and on top of that my ctsm with soil NOx version was implemented in cheyenne and had a hard time to make it run in derecho. I also need to admit that github is not my forte and need some guidance here....

So, I started transfering the soil NOx code while addressing the previous comments from @wwieder and @samsrabin (eg remove my 'mvm' notes, etc) in a new version that works in derecho /glade/derecho/scratch/mariavm/derecho_ctsm/ Is that a good approach? I see @slevis-lmwg requested some additional modifications, so are those on top of @wwieder and @samsrabin comments or those previous ones have already been addressed?

I still need to open the a PR on CMEPS for passing these fluxes through the coupler but wanted to get everything sorted in ctsm_soilnox sorted first but should I go ahead and request it?

Thanks a lot!

Maria

@slevis-lmwg
Copy link
Contributor

@mvalmartin please view my two suggestions as additional to prior comments, though I entered this discussion late and do not necessarily know what's going on. And my review's 5-point task list indicates requirements for getting code into "main" without indicating or making assumptions about who will complete each step.

Seems fine to me that you made a new version that works on derecho, though keep in mind that /scratch has a scrubber, so you may lose work on that disk. That version will need to come to github to this or a follow-up PR, right?

@mvalmartin
Copy link
Author

Hi @wwieder, @samsrabin, @slevis-lmwg, @ekluzek and @tilmes, I have implemented the soil NOx scheme in a new version of ctsm as I couldn't make my old version run in derecho for testing. It is in here: /glade/work/mariavm/derecho_ctsm_soilnox

I have also incorporated most of your modifications except 'creating a namelist variable to define the method by which N gases are calculated (e.g., Koven2013 vs. Valmartin2023)' as that was beyond my current expertise and time available to learn!

The only thing left is to couple soil_nox_atmos from CTSM to CAM.

Do you need to push this version to github?

Thanks a lot and apologies for the delay!

Maria

@samsrabin
Copy link
Member

@mvalmartin It sounds like you have a new branch, right? And the one in this PR is not being used anymore? In that case, I think it would be best to close this pull request, then submit a new one with your new branch (including a link to this PR).

@wwieder
Copy link
Contributor

wwieder commented May 22, 2024

I'd agree, letting us see your update would be helpful. That's likely easiest if you just open a new PR.

@wwieder wwieder added the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label May 22, 2024
@wwieder
Copy link
Contributor

wwieder commented May 23, 2024

It seems worth including some of our email discussion with @lkemmons & @tilmes. Louisa wrote:
Hi Will, I think it would be great if we could get the soil NO emissions included in CESM3. It sounds like Maria is just stuck with making a pull request, etc.
Can we help get this done?

@wwieder responded:
Hi Louisa, This is a scientific feature I'd like to be able to support. We can try, but I think it's unlikely we'll be able to integrate a yet to be completed PR by a June 30th deadline with other priorities that the CTSM software team is trying to tackle. Moreover, this PR also needs to be scientifically vetted (e.g. does the change in soil N cycling feedback onto the terrestrial C cycle, which I don't think Maria ever evaluated). This isn't a show stopper, but it will take some additional time.

If you can provide SE support, especially for the technical bits that Maria's struggling with, that would be excellent and certainly help us out. Could Francis (of someone else) help move this PR through the pipeline?

I think the prioritization of somewhat comes down to the CAM-Chem group and Dave (@dlawrenncar). Can we delay the science freeze to accommodate this request? How high a priority is it for the science you'd like to do with CESM3.0 (as opposed to CESM3.1)?

@wwieder
Copy link
Contributor

wwieder commented May 23, 2024

After further discussing this PR at our SE meeting today I think it's clear that the LMWG will not be able to get this PR onto main by the June 30 code freeze. I'm happy to outline what needs to happen to bring in this work and make it available to the community, but it's a larger discussion about deadlines and priorities for the default configuration of CESM3.

@wwieder wwieder removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label May 23, 2024
@lkemmons
Copy link

After further discussing this PR at our SE meeting today I think it's clear that the LMWG will not be able to get this PR onto main by the June 30 code freeze. I'm happy to outline what needs to happen to bring in this work and make it available to the community, but it's a larger discussion about deadlines and priorities for the default configuration of CESM3.

Thanks for the update. We will work on implementing and fully evaluating this after the freeze.

@ekluzek
Copy link
Collaborator

ekluzek commented Jun 12, 2024

I had previously promised to add the page on how to add namelist items to CLM. I just found where that is, so I'm posting here in case it's helpful. As one thing we had talked about was putting namelist control around this new feature.

https://wiki.ucar.edu/display/ccsm/Adding+New+Namelist+Items+to+CLM

@wwieder
Copy link
Contributor

wwieder commented Jun 17, 2024

@lkemmons thanks for offering to pick up this work. I don't know if it's easiest to merge the latest work you referenced in your comment here into this PR, or close this PR and open a new one? Either way, it would be nice to see where comments from the previous code review were addressed.

Separately, I want to emphasize that besides the logistics of merging the PR we also need to:

  1. link to a CMEPs PR that passes soil NO fluxes to the mediator
  2. Bring the changes across in to the Matrix-CN solution.
  3. Scientifically evaluate the changes on terrestrial C and and N cycles from these changes.

Accomplishing these additional tasks seems unlikely on a July 31 timeline, but we can't even begin to evaluate this until we see an updated PR on the core of the soil BGC work.

@ekluzek
Copy link
Collaborator

ekluzek commented Jul 25, 2024

@lkemmons you were going to work on this one have you made any progress on it? This seems like something that will need to come in later after the current "chill" activity happens.

@lkemmons
Copy link

@ekluzek Sorry, I have not found time to work on this.
I think Maria's code (/glade/work/mariavm/derecho_ctsm_soilnox/) is in ctsm5.1.dev157.
Can I start a new PR with it? Or should I incorporate those changes into a newer version?

@samsrabin samsrabin added science Enhancement to or bug impacting science and removed enh - new science labels Aug 8, 2024
@ekluzek
Copy link
Collaborator

ekluzek commented Feb 4, 2025

@wwieder I'm taking the lack of urgency in this for CESM3.0 and without there being dedicated work being done on this -- that should be slated for a ctsm7.0.0 milestone. Does that seem correct to you?

@ekluzek ekluzek added the priority: low Background task that doesn't need to be done right away. label Feb 4, 2025
@lkemmons
Copy link

lkemmons commented Feb 4, 2025

@mvalmartin plans to visit NCAR/ACOM this summer and complete this implementation then.

@wwieder wwieder mentioned this pull request Feb 5, 2025
@wwieder
Copy link
Contributor

wwieder commented Jul 8, 2025

@mvalmartin is back in Boulder and working on this!
I wonder if it's most efficient to merge this PR with #2992 so all of our soil N changes are in a single branch.
@slevis-lmwg can you help Maria with some github issues and make suggestions for adding namelist control over this PR so we can bring it on b4b with the changes off and then evaluate how it influences land simulations down the road?

@slevis-lmwg
Copy link
Contributor

@mvalmartin is back in Boulder and working on this! I wonder if it's most efficient to merge this PR with #2992 so all of our soil N changes are in a single branch. @slevis-lmwg can you help Maria with some github issues and make suggestions for adding namelist control over this PR so we can bring it on b4b with the changes off and then evaluate how it influences land simulations down the road?

Yes, I'm happy to help.
@mvalmartin I shared my calendar with you, so that you may find a good time for us to meet when you're ready.

@wwieder
Copy link
Contributor

wwieder commented Jul 8, 2025

I also want to make clear that although I hope to have this available for the CLM6 release, it will be off by default in CESM3, as the code freeze for science changes has already passed.

@slevis-lmwg
Copy link
Contributor

This PR is replaced by #3341 and, I think, could be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement new capability or improved behavior of existing capability priority: low Background task that doesn't need to be done right away. science Enhancement to or bug impacting science

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants