-
Notifications
You must be signed in to change notification settings - Fork 317
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
Add soil tillage for crops #2040
Conversation
Each crop column should only have one patch, so by only calling these for crop columns, we can simplify things considerably.
"Throw error during namelist build if tillage is called with FATES"
Update FATES tests to double precision This pull request updates the fates tests to set the output precision to double precision. The usermod fates_sp is similarly updated.
@slevis-lmwg, can I look on your calendar to schedule a re-review? |
b4b changes to Python scripts, history lists, tech note, and clm_time_manager. * Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081) * Rework master_list* files etc. (ESCOMP#2087) * Fixes to methane Tech Note (ESCOMP#2091) * Add is_doy_in_interval() function (ESCOMP#2158) * Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125) Closes issues: * Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079) * Rework master_list_(no)?fates.rst? (ESCOMP#2083) * conda run -n can fail if a conda environment is already active (ESCOMP#2109) * conda fails to load for SystemTests (ESCOMP#2111)
Yes, please do. My calendar is very busy, so I would prefer no sooner than
Thursday afternoon.
…On Mon, Oct 2, 2023, 12:02 PM Sam Rabin ***@***.***> wrote:
@slevis-lmwg <https://github.com/slevis-lmwg>, can I look on your
calendar to schedule a re-review?
—
Reply to this email directly, view it on GitHub
<#2040 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AINPYFCJXEEQ3W3WMKZXZPTX5MFWBAVCNFSM6AAAAAAZQY7D26VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBTGU4TMNZRGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Tasks from re-review with @slevis-lmwg:
Additional tasks: |
Various BFB fixes and updates Purpose/description of changes ------------------------------ the default comes in a later tag (slevis) Regular and python testing passed. Does not change answers relative to dev158.
I've figured out the reason my runs showed such a small tillage effect relative to Mike's! Turns out, I had incorrectly ordered the dimensions of the Now that this is fixed (v2), the tillage effect is about as strong as it was with Mike's code. |
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.
@samsrabin and I went over updates since I last reviewed this PR. Sam R will change a couple of existing "crop" tests to "till" tests and beyond that this PR is ready.
Description of changes
This PR brings in the tillage code written by Sam Levis and Michael Graham and used in Graham et al. (2021, ERL, doi:10.1088/1748-9326/abe6c6). Low- and high-intensity tillage here work by increasing the decomposition rate of different soil carbon pools. These "decomposition multipliers" vary based on soil pool and how long it's been since the crop was planted; they are set with new paramfile variables
till_decompk_multipliers
andmimics_till_decompk_multipliers
.NOTE: This PR prevents tillage from actually being enabled. The reasoning is that we want to get the new parameters into the paramfile soon, but scientific testing will take a while.
Specific notes
Contributors other than yourself, if any: Michael Graham (@mwgraham), Sam Levis (@slevis-lmwg), Danica Lombardozzi (@danicalombardozzi)
CTSM Issues Fixed:
Are answers expected to change? No
Any User Interface Changes (namelist or namelist defaults changes)?
tillage_mode
with optionsoff
(default),low
, andhigh
.use_original_tillage_phases
(false by default; see "Substantive changes" below).max_tillage_depth
.Testing performed, if any:
aux_clm
tests OK (NLFAIL) except for expected failures. Baseline saved astillage.d43c6178c
.Substantive changes from original @slevis-lmwg / @mwgraham code
idpp
), using instead a calculation that seems incorrect for growing seasons that cross over into a new calendar year. This version usesidpp
instead. However, the old version can be used by settinguse_original_tillage_phases
to true.max_tillage_depth
is 26 cm. This was the intention of the original code, but it accidentally used 32 cm.Limitations
Questions
Substantive
@wwieder: Should I extend this to work with MIMICS decomposition? Doing so would require some scientific thinking and probably literature review.Yes; done.Should I extend this to work with FATES? I think the only reason I disabled that is because I'm unsure how/whether coarse woody debris, which FATES handles, should be affected by tillage.Yes.Should I bring in (an option to enable) GDP dependence, as was in the original code but unused? Unfortunately the "developed country" version uses hard-coded dates to determine decomposition multiplier; we'd surely want to generalize that but it'd require a discussion.Not doing this for now, as it seems to have only been an initial attempt.Technical
Should I move the 54 decomposition multipliers (currently hard-coded) to the parameters file?Yes; done.Is there a simpler way than this to get the active patch associated with a given crop column?Seemingly no.testlist_clm.xml
that uses thetill
testdef? And/or, should I edit one of theaux_clm
tests to use it instead ofcrop
?Remaining work
aux_clm
till
testdef.