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

Remove the CN (non CENTURY) version of Carbon-Nitrogen soil biogeochemistry code and its testing #1356

Open
ekluzek opened this issue Apr 22, 2021 · 16 comments
Assignees
Labels
code health improving internal code structure to make easier to maintain (sustainability)

Comments

@ekluzek
Copy link
Collaborator

ekluzek commented Apr 22, 2021

Remove the option to run the non-CENTURY and vertically resolved BGC code. This code isn't really being used, validated, or kept up to date. None of the current supported versions of the physics use it the default (clm4_5, clm5_0, nor clm5_1). There are also concerns for its scientific validity especially as compared to the full BGC version. The only advantage is it's simpler and faster, but because it isn't and hasn't been tuned in any of the current model physics versions it likely is just adding complexity.

This could be as simple as removing the following file from soilbiogeochemistry:

SoilBiogeochemDecompCascadeCNMod.F90

as well as the "CN" compsets and tests:

I2000Clm50Cn
I1850Clm45Cn

ERP_D_Ld9" grid="f19_g17" compset="I2000Clm50Cn" testmods="clm/drydepnomegan
ERP_P36x2_D_Ld5" grid="f10_f10_mg37" compset="I1850Clm45Cn" testmods="clm/default
ERP_P36x2_D_Ld5" grid="f10_f10_mg37" compset="I2000Clm50Cn" testmods="clm/default
SMS" grid="f19_g17" compset="I2000Clm50Cn" testmods="clm/default
SMS_D_Ld1" grid="f19_g17" compset="I1850Clm45Cn" testmods="clm/default
SMS_P48x1_D_Ld5" grid="f10_f10_mg37" compset="I2000Clm50Cn" testmods="clm/default

Doing that would be quite straightforward.

@ekluzek ekluzek added the code health improving internal code structure to make easier to maintain (sustainability) label Apr 22, 2021
@ekluzek
Copy link
Collaborator Author

ekluzek commented Apr 22, 2021

What gets more involved is removing the "cn" option from build-namelist, and also deciding if we should keep the namelist variables: use_lch4, use_nitrif_denitrif, use_vertsoilc, use_century_decomp. From what @wwieder is recommending it sounds like use_century_decomp and use_vertsoilc should certainly be removed as an option and hardcoded to be assumed on. I suspect that doing that might make other code simplifications possible. There's a little bit of code that goes into allowing both single layer Carbon and multi-layer Carbon layers for example. There are also at least 22 constants on the parameter file that could be removed if CN was removed.

I'm not sure if the other two namelist items (use_lch4, and use_nitrif_denitrif) would be useful to keep around or not. If we did want to keep them around, we should add a test for them (since the CN tests would be removed). The only one of them invoked in our tests right now is use_lch4 which is turned off for the testmod smallville_dynlakes_monthly. There is also a test mod for non-vertically resolved and non-nirification-dentrification NoVSNoNI so that should likely be modified to cover whatever namelist items are left to still invoke.

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

wwieder commented Apr 22, 2021

This could take some time to unwind (and for me to wrap my head around). Is this the start of implementing the revisions of the namelist discussion we were having with @rgknox a few months back? I'm not sure where this discussion or notes are.

@ekluzek
Copy link
Collaborator Author

ekluzek commented Apr 22, 2021

@wwieder I put the next label so that we could discuss next week. And no this unfortunately is separate from the namelist discussion we had. The start of that is issue #942. The document is here:

https://docs.google.com/spreadsheets/d/1wwI_RuQ87FNk4PdrSLtlplisEMTRqDCxFeqG6Wbxt_c

The one thing that might be entwined with that is that if we remove the use_century_decomp namelist item it should be replaced with an option to control soildecomp that would have century as the only option.

@billsacks billsacks changed the title Remove the CN (non CENTURY) version of Carbon-Nitrogen soil biogeochemistry code and it's testing Remove the CN (non CENTURY) version of Carbon-Nitrogen soil biogeochemistry code and its testing Apr 22, 2021
@ekluzek
Copy link
Collaborator Author

ekluzek commented May 6, 2021

In a meeting @wwieder thought that we probably should hardcode methane and nitrif_denitif on, when we remove the CN option. There doesn't seem a reason to have the ability to turn it off.

@billsacks billsacks removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label May 6, 2021
@ekluzek
Copy link
Collaborator Author

ekluzek commented Jun 3, 2021

We are going to ramp up the need to do this, so that @slevisconsulting doesn't have to work on the old CN code for changes coming in. His first version will just make CN tests expected fails.

@ekluzek
Copy link
Collaborator Author

ekluzek commented Jun 3, 2021

OK in ctsm5.1.dev043 these tests will fail:

ERP_D_Ld9.f19_g17.I2000Clm50Cn.cheyenne_intel.clm-drydepnomegan
ERP_P36x2_D_Ld5.f10_f10_mg37.I1850Clm45Cn.cheyenne_intel.clm-default
ERP_P36x2_D_Ld5.f10_f10_mg37.I2000Clm50Cn.cheyenne_gnu.clm-default
ERP_P36x2_D_Ld5.f10_f10_mg37.I2000Clm50Cn.cheyenne_intel.clm-default
SMS.f19_g17.I2000Clm50Cn.cheyenne_intel.clm-default
SMS_D_Ld1.f19_g17.I1850Clm45Cn.izumi_gnu.clm-default

The error that's shown with the DEBUG tests is a subscript error as follows. Production tests just die with an abort.

169:forrtl: severe (408): fort: (3): Subscript #3 of the array DWT_FROOTC_TO_LITR_C_COL has value -9 which is less than the lower bound of 1
169:
169:Image PC Routine Line Source
169:cesm.exe 0000000004677B6F Unknown Unknown Unknown
169:cesm.exe 00000000034AC66A cnvegcarbonfluxty 2900 CNVegCarbonFluxType.F90
169:cesm.exe 00000000033DACE8 cnvegcarbonfluxty 398 CNVegCarbonFluxType.F90
169:cesm.exe 00000000013D221B cnvegetationfacad 247 CNVegetationFacade.F90
169:cesm.exe 00000000009AC892 clm_instmod_mp_cl 430 clm_instMod.F90
169:cesm.exe 000000000099E6A3 clm_initializemod 357 clm_initializeMod.F90

@ekluzek
Copy link
Collaborator Author

ekluzek commented Jun 3, 2021

I think it's OK for those tests to just be dropped since there are other drydepnomegan tests as well as threading ERP tests. The others test mods are just default configurations.

ekluzek added a commit to olyson/ctsm that referenced this issue Jun 10, 2021
@ekluzek
Copy link
Collaborator Author

ekluzek commented Jun 11, 2021

I've done partial work on this. I need to discuss this again for the rest. I'm not sure about getting rid of use_lch4, use_nitrif_denitrif now because of Fates. And use_vertsoilc seems to be useful in issue #1395. I also think I should start replacing use_century_decomp with soildecomp_method that is a character string, that then uses an integer internally.

@ekluzek ekluzek added the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Jun 11, 2021
@ekluzek ekluzek self-assigned this Jun 11, 2021
@wwieder
Copy link
Contributor

wwieder commented Jun 12, 2021

@ekluzek this makes sense to me. let's discuss farther after the CESM workshop.

@billsacks
Copy link
Member

From today's ctsm-software:

  • FATES uses use_nitrif_denitrif true (so okay to remove that namelist flag) (though nitrogen cycle is off, so it's irrelevant). @wwieder notes that we may eventually want to allow ECA, though it seems like that would be handled differently: nitrif/denitrif is still on, but the method for handling it would change.
  • FATES turns off methane (use_lch4) by default; we'll keep this flag available
  • use_vertsoilc: Main reason @wwieder can see for turning it off is to speed things up. @ekluzek points out that it can be helpful to find problems (like SASU crashes when altering SOM C:N ratio parameters #1395). Overall feeling is that we can hard-code this to true.
  • use_century_decomp: Should start introducing soil_decomp_method – for now bgc, but later can include mimics. For a name, can use something like BGCKoven2013. Or should we move away from the "BGC" name, using something like CENTURYKoven2013? We like the latter.

@billsacks billsacks removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Jul 1, 2021
@billsacks
Copy link
Member

Based on email discussion with @rgknox @ekluzek @ckoven - methane will need to be on if use_nitrif_dentrif is on. So if our plan is to hard-code / implicitly assume use_nitrif_dentrif to true, then we should also hard-code / implicitly assume methane is on.

@ckoven says that this block should be changed to do the endrun rather than setting things to 0:

else
! NITRIF_DENITRIF requires Methane model to be active,
! otherwise diffusivity will be zeroed out here. EBK CDK 10/18/2011
anaerobic_frac(c,j) = 0._r8
diffus (c,j) = 0._r8
!call endrun(msg=' ERROR: NITRIF_DENITRIF requires Methane model to be active'//errMsg(sourcefile, __LINE__) )
end if

though that will be irrelevant if we always turn methane on.

@ekluzek
Copy link
Collaborator Author

ekluzek commented Aug 5, 2021

The endrun can be added back in, but only if it allows FATES to work.

@ekluzek
Copy link
Collaborator Author

ekluzek commented Aug 19, 2021

So Fates cases default to having use_nitrif_denitrif=.false. and it changes answers if you turn it on. So I'm going to file that as a separate issue, and we'll remove use_nitrif_denitrif as a separate thing.

@ekluzek
Copy link
Collaborator Author

ekluzek commented Aug 26, 2021

So this is problematic for the test ERS_Lm25.1x1_smallvilleIA.IHistClm50BgcCropQianRs.cheyenne_gnu.clm-smallville_dynlakes_monthly which turns methane off, because dynamic lakes can't run with methane on. One way to do this would be to create a Sp test for this case, but this specific test is built around smallville which requires both BGC and Crop. So I think the way around this for now is to allow methane and nitrif_denitrif to be off for this case, but add a warning about it. Later we can replace this test with a Sp version of it.

@slevis-lmwg
Copy link
Contributor

Copying written comments from today's ctsm software meeting so that we do not forget:

  • In a meeting @wwieder thought that we probably should hardcode methane and nitrif_denitif on
    This does mean that Methane will also be hardcoded to on.
  • We couldn't do that before because FATES had these both as off. But, now that's changed.
    So they could be hardcoded on for FATES as well at this point.

@ekluzek
Copy link
Collaborator Author

ekluzek commented Aug 10, 2023

Oh, I just saw a note above Aug 25, 2021 that says that a specific test would have a problem. The underlying problem is that dynamic lakes can't run with methane on? If that's the case we could do BGC and dynamic lakes, which sounds like a problem. The suggestion at the end of the issue is that we turn off the gridcell methane balance checks (which we have already done), but there is also an issue for time-step 0 (#1868) for the ch4 balance check error there. This could be resolved when #2084 comes in, or we could do something explicit for it in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health improving internal code structure to make easier to maintain (sustainability)
Projects
None yet
Development

No branches or pull requests

4 participants