SIP328 fix wood storage bookkeeping so handling of plantWoodCStorageDelta is consistent#330
SIP328 fix wood storage bookkeeping so handling of plantWoodCStorageDelta is consistent#330dlebauer wants to merge 2 commits intoPecanProject:masterfrom
plantWoodCStorageDelta is consistent#330Conversation
Route harvest removals through structural wood and positive wood-storage pools separately. Harvest now updates plantWoodCStorageDelta through a dedicated eventWoodStorageDelta flux so a full aboveground harvest removes positive storage instead of leaving reported plantWoodC behind. Add plant wood helper functions for total reported wood and storage-backed process carbon. Output reporting now uses the total helper, while wood maintenance respiration and wood turnover use storage-backed carbon so negative storage debt does not create negative process fluxes. Add focused modeling tests for output semantics, negative-storage respiration and turnover, positive-storage harvest removal, negative-storage harvest debt handling, and carbon balance across the event update.
There was a problem hiding this comment.
Pull request overview
This PR makes SIPNET’s wood carbon bookkeeping consistent when plantWoodCStorageDelta is present, aligning harvest handling, wood respiration/turnover calculations, and sipnet.out reporting semantics to better satisfy mass-balance and avoid nonphysical negative wood flux behavior.
Changes:
- Add helper accessors for “total wood” and “storage-backed wood” and use them in
sipnet.out, wood respiration, and wood turnover. - Update HARVEST event logic to explicitly remove positive
plantWoodCStorageDelta(while leaving negative storage debt visible) and apply storage-delta updates to the state. - Add focused unit tests covering output semantics and harvest scenarios with positive/negative storage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/sipnet/test_modeling/testWoodStorage.c |
New unit tests for output semantics, flux behavior under negative storage, and harvest routing of positive/negative storage. |
tests/sipnet/test_modeling/Makefile |
Adds the new wood storage test to the modeling test suite and cleans its artifacts. |
src/sipnet/state.h |
Declares new helper functions and adds an event flux for storage-delta routing. |
src/sipnet/state.c |
Implements helper functions for total wood and “storage-backed” wood carbon. |
src/sipnet/sipnet.c |
Uses helpers in sipnet.out reporting and in wood respiration/turnover flux calculations. |
src/sipnet/events.c |
Updates harvest to split structural wood vs. positive storage removal and applies storage-delta event flux to state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fluxes.eventSoilC += soilAdd / climLen; | ||
| fluxes.eventLeafC += leafDelta / climLen; | ||
| fluxes.eventWoodC += woodDelta / climLen; | ||
| fluxes.eventWoodStorageDelta += storageWoodDelta / climLen; | ||
| fluxes.eventFineRootC += fineDelta / climLen; |
There was a problem hiding this comment.
fluxes.eventWoodStorageDelta is introduced/updated for harvest, but resetEventFluxes() doesn’t currently reset it. If processEvents() is ever called without a preceding resetFluxes() (several unit tests call processEvents() directly), the storage-delta event flux can carry over between timesteps/tests and corrupt pool updates. Add fluxes.eventWoodStorageDelta = 0.0; to resetEventFluxes() alongside the other event flux resets.
| fluxes.eventLeafC += leafDelta / climLen; | ||
| fluxes.eventWoodC += woodDelta / climLen; | ||
| fluxes.eventWoodStorageDelta += storageWoodDelta / climLen; | ||
| fluxes.eventFineRootC += fineDelta / climLen; |
There was a problem hiding this comment.
Harvest now updates plantWoodCStorageDelta via fluxes.eventWoodStorageDelta, but the writeEventOut() call for HARVEST doesn’t include this delta. Since events.out is used for debugging/testing and is intended to list all state deltas applied by an event, this omission makes harvest rows incomplete/misleading (especially when positive storage is removed). Include fluxes.eventWoodStorageDelta in the parameters written for harvest events.
| params.baseVegResp = 1.0; | ||
| params.vegRespQ10 = 1.0; | ||
| params.woodTurnoverRate = 0.5; | ||
| params.coarseRootTurnoverRate = 0.0; | ||
| params.fineRootTurnoverRate = 0.0; | ||
| params.coarseRootAllocation = 0.0; | ||
| params.fineRootAllocation = 0.0; | ||
| params.woodAllocation = 0.0; | ||
| params.leafCN = 20.0; | ||
| params.woodCN = 10.0; | ||
| params.fineRootCN = 30.0; | ||
|
|
||
| envi = (Envi){0}; | ||
| envi.soilC = 10.0; | ||
| envi.soilWater = 5.0; | ||
|
|
||
| initTrackers(); | ||
| resetFluxes(); | ||
| initBalanceTracker(); | ||
| } |
There was a problem hiding this comment.
initTrackers() computes trackers.soilWetnessFrac = envi.soilWater / params.soilWHC, but this test setup never initializes params.soilWHC. If it remains 0 (common in isolated unit tests), this produces Inf/NaN and can make the test output nondeterministic. Initialize params.soilWHC (and any other params used by initTrackers() in this test) to a non-zero value before calling initTrackers().
Summary
plantWoodCStorageDeltaconsistently.Without this fix, I was unable to find plausible parameterization that produced sensible AGB values following double counting of woodstorage that was fixed in #248.
How was this change tested?
Added focused tests for:
sipnet.outreportingplantWoodC = structural wood + storage deltanppStorage = plantWoodCStorageDeltaStill useful follow-up coverage:
plantWoodCStorageDeltaStill TODO
Reproduction steps
Related issues
For #324, this fixes
For #325, this ensures that negative wood storage does not make wood-based process fluxes negative.
Checklist
docs/user-guide/model-outputs.mdthat textplantWoodCis reported total wood (plantWoodC + nppStorage) and structural wood isplantWoodC - nppStorage.docs/CHANGELOG.mdupdated with noteworthy changesclang-format(rungit clang-formatif needed)