SIP287 Add C Saturation - #301
Conversation
Alomir
left a comment
There was a problem hiding this comment.
New flag option looks good! (Other than the one issue in restart.c)
Merging from master
|
Smoke and Unit test output. |
There was a problem hiding this comment.
Pull request overview
Implements optional soil carbon saturation (issue #287) by adding a new carbonSaturation model flag, a soilCSaturation parameter, and a saturation-fraction-based rerouting of root-loss and litter-to-soil carbon back into the litter pool in updatePoolsForSoil(). The flag is wired through Context, CLI, restart serialization, and validation (it requires litterPool), and is exercised by a new unit test and existing smoke runs.
Changes:
- Adds
carbon-saturationCLI/model flag (Context, CLI map, restart schema, validation) and registers a new optionalsoilCSaturationparameter. - Modifies
updatePoolsForSoil()to partition soil-bound carbon inputs byenvi.soilC / params.soilCSaturationbetween soil and litter pools when the flag is on. - Adds
testCarbonSaturation.cunit test plus smoke-test config/param updates.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sipnet/sipnet.c | New saturation branch in updatePoolsForSoil() and parameter registration. |
| src/sipnet/state.h | Adds soilCSaturation param field (and a stray blank line in FluxVars). |
| src/sipnet/cli.c, cli.h | Adds --carbon-saturation flag, arg map entry, usage text, bumps NUM_FLAG_OPTIONS. |
| src/common/context.h, context.c | Adds carbonSaturation Context field, initializer, and validateContext() dependency on litterPool. |
| src/sipnet/restart.c | Adds the flag to restart schema, serialization, and compatibility check. |
| tests/sipnet/test_modeling/testCarbonSaturation.c, Makefile | New unit test covering four input/saturation combinations. |
| tests/smoke/{niwot,russell_*}/sipnet.config, sipnet.param | Adds default-off CARBON_SATURATION line and soilCSaturation 1.0 param entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| double saturationFraction = envi.soilC / params.soilCSaturation; | ||
| double soilInputs = | ||
| fluxes.coarseRootLoss + fluxes.fineRootLoss + fluxes.litterToSoil; | ||
|
|
||
| envi.litterC += (fluxes.woodLitter + fluxes.leafLitter + | ||
| (soilInputs * saturationFraction) - fluxes.litterToSoil - | ||
| fluxes.rLitter - fluxes.litterMethane) * | ||
| climate->length; | ||
|
|
||
| envi.soilC += (soilInputs * (1 - saturationFraction) - fluxes.rSoil - | ||
| fluxes.soilMethane) * | ||
| climate->length; |
There was a problem hiding this comment.
This sounds like something to address
There was a problem hiding this comment.
Addressed, saturationFraction has been clamped to 0-1
| soilInputs = | ||
| fluxes.coarseRootLoss + fluxes.fineRootLoss + fluxes.litterToSoil; | ||
| saturationFraction = expSoilC / params.soilCSaturation; | ||
|
|
||
| expLitterC += (fluxes.woodLitter + fluxes.leafLitter + | ||
| (soilInputs * saturationFraction) - fluxes.litterToSoil - | ||
| fluxes.rLitter - fluxes.litterMethane) * | ||
| climate->length; | ||
|
|
||
| expSoilC += (soilInputs * (1 - saturationFraction) - fluxes.rSoil - | ||
| fluxes.soilMethane) * | ||
| climate->length; | ||
|
|
||
| updatePoolsForSoil(); | ||
| status |= checkSoil(expSoilC); | ||
| status |= checkLitter(expLitterC); |
There was a problem hiding this comment.
Revised and included test for envi.soilC > params.soilCSaturation scenario
dlebauer
left a comment
There was a problem hiding this comment.
This looks great, glad to have this feature and look forward to seeing whether and when it improves model performance.
I'm about to start a code review but before merging, could you please update the documentation?
dlebauer
left a comment
There was a problem hiding this comment.
Thanks for adding the docs. I made a few comments related to improving clarity.
Adding the clamp to the code as requested by Mike and Copilot will make code consistent with docs, and avoid negative fluxes.
| | $Q_{10s}$ | soilRespQ10 | Soil respiration Q10 | unitless | scalar determining effect of temp on soil respiration | | ||
| | $D_{\text{moisture}}$ | soilRespMoistEffect | scalar determining effect of moisture on soil resp. | unitless | | | ||
| | $f_{\text{till}}$ | tillageEff | Effect of tillage on decomposition that exponentially decays over time | fraction | Documented in model structure; event-level term in `events.in` | | ||
| | $f_{\text{C,soil,saturation}}$ | soilCSaturation | Maximum amount of carbon that can be stabilized in the soil determined by soil texture | $\text{g C} \cdot \text{m}^{-2} \text{ ground area}$ | | |
There was a problem hiding this comment.
f is reserved for unitless "fraction" parameters.
Something like C_soil,sat, or C^*_soil
There was a problem hiding this comment.
Revised to C_soil,saturation
| If soil carbon saturation is enabled, only a fraction of the total carbon input will be added to the soil. The amount added to the soil carbon pool is a function of the proximity of the pool to the soil carbon saturation limit. As the soil carbon pool aqcuires more carbon and approaches its saturation limit, the amount of carbon stored in the pool decreases. This represents the physical and chemical constraints of the mineralogical surfaces of soil to stabilize carbon for long-term storage. The remaining carbon that is not stabilized in the soil carbon pool due to saturation constraints will be returned to the litter carbon pool to act as fast-turnover carbon \eqref{eq:soil_carbon_to_litter}. | ||
|
|
||
| \begin{equation} | ||
| \frac{dC_\text{soil}}{dt} = F^C_{\text{soil}} \cdot (1 - \frac{C_{\text{soil}}}{f_{\text{C,soil,saturation}}}) - R_{\text{soil}} - F^C_{\text{CH}_4\text{,soil}} |
There was a problem hiding this comment.
F^C_{\text{soil}} it is used earlier to represent “total carbon input to soil pool,” not as an unstabilized return flux, please confirm that it is the correct quantity to use here.
There was a problem hiding this comment.
Yes, this is the correct term. The inputs did not change, they are just getting multiplied by (1-saturationFraction). Therefore, F^C_{\text{soil}} remains the same. The term is also correct in the envi.litterC calculation because it is the remainder of the fraction that gets redirected to the litter pool. If F^C_{\text{soil}} * (1-saturationFraction) C goes to the soil pool, then F^C_{\text{soil}} * (saturationFraction) C goes to the litter pool.
|
All comments have been addressed, ready for another review. |
…turation Merge from master
Add restriction for 'carbon-saturation' requiring 'litter-pool'.
dlebauer
left a comment
There was a problem hiding this comment.
Thanks, this looks great. I reviewed the documentation and made a few suggestions.
…om/PecanProject/sipnet into SIP287-Implement-Carbon-Saturation
|
As @dlebauer is away for several weeks, I'm going to merge this in. I believe all of the doc comments have been addressed; any remaining tweaks can be done in a separate PR |
|
This PR is not showing up in the closed PRs list - hoping this comment forces a reindex. |
Summary
Implementing soil carbon saturation by creating a flag, adding a new parameter, modifying fluxes into soil organic carbon pool, adding new flux from soil organic carbon pool to litter pool, and updating documentation. Motivation for this change includes evaluating model soil carbon dynamics with and without an explicit maximum limit for long-term storage.
How was this change tested?
List steps taken to test this change, with appropriate outputs if applicable
If changes are needed for any
sipnet.outfiles in thetest/smokesubdirectories, then include output fromtools/smoke_check.pyby running the commands below and pasting the output at the end of this PR. Note thatthis must be run BEFORE submitting changes to any
sipnet.outfiles.Run
python tools/smoke_check.py helpfor more info.Related issues
Checklist
docs/CHANGELOG.mdupdated with noteworthy changesclang-format(rungit clang-formatif needed)Note: See CONTRIBUTING.md for additional guidance. This repository uses automated formatting checks; if the pre-commit hook blocks your commit, run
git clang-formatto format staged changes.