SIP350 Update carbon accounting + plant mortality

[Alomir]

Summary

• What: Revamp handling of the C pseudo-storage pool
• Motivation: Current handling is broken.

NOTE: THIS REPLACES #353

How was this change tested?

• Existing unit tests updated
• New unit tests added to cover new functionality
• Smoke tests run and updated; diff results below

smoke_check.txt

Related issues

• Fixes Revamp biomass N handling #350, Insufficient carbon for growth should cause mortality, and fix C balance #325

Checklist

• Related issues are listed above. PRs without an approved, related issue may not get reviewed.
• PR title has the issue number in it ("[#] <concise description of proposed change>")
• Tests added/updated for new features (if applicable)
• Documentation updated (if applicable)
• docs/CHANGELOG.md updated with noteworthy changes
• Code formatted with clang-format (run git clang-format if 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-format to format staged changes.

[Alomir]

@copilot please open a PR to update the unit tests in tests/sipnet to cover these changes. Create new similar tests if needed.

[Copilot]

@Alomir I've opened a new pull request, #360, to work on those changes. Once the pull request is ready, I'll request review from you.

[Copilot]

Pull request overview

This PR revamps SIPNET’s carbon “accounting/buffer” handling (renaming plantWoodCStorageDelta → plantCAccountingDelta), adds a plant mortality/reemergence tracker with event output, updates restart/debug-log serialization to include the new tracker and event fields, and refreshes the Python viewer tools plus unit/smoke coverage accordingly.

Changes:

• Rework biomass C flux calculations to account for negative mean NPP and to use the carbon accounting delta consistently; add plant mortality detection and plantdeath event output.
• Extend restart/debug logging schemas (survival tracker + expanded event trackers) and update unit tests/new modeling tests.
• Refactor sipnet-view / sipnet-debug-view tooling (shared arg parser, plot controls, toolbar hover display), plus smoke harness updates (optional debug-log runs).

Reviewed changes

Copilot reviewed 30 out of 34 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
tools/sipnet_view.py	Adds configurable viewer window title; switches to shared default CLI parser.
tools/sipnet_view_common.py	Adds shared CLI parser helper, plot display controls, and custom toolbar hover reporting.
tools/sipnet_debug_view.py	Adds configurable window title; uses shared default CLI parser; minor perf copy before sorting/merging.
tests/smoke/russell_3/events.out	Updates expected computed event outputs after new accounting/mortality behavior.
tests/smoke/russell_2/events.out	Updates expected computed event outputs after new accounting/mortality behavior.
tests/smoke/russell_1/sipnet.config	Updates recorded “Final config” timestamp from smoke run.
tests/smoke/russell_1/events.out	Updates expected computed event outputs after new accounting/mortality behavior.
tests/smoke/run_smoke.sh	Adds optional debug-log generation mode for smoke runs.
tests/sipnet/test_sipnet_infrastructure/testDebugLogFiles.c	Updates debug-log header expectations; adds init/cleanup to avoid leaving modified files behind.
tests/sipnet/test_restart_infrastructure/mock_state.c	Updates restart schema drift mocking to use renamed accounting field.
tests/sipnet/test_modeling/testPlantMortality.c	New unit test suite for mortality + pool transfers (with/without litter and nitrogen).
tests/sipnet/test_modeling/testNitrogenCycle.c	Adds focused unit test for the new incremental N resorption flux helper.
tests/sipnet/test_modeling/testFluxCalculations.c	New unit tests for wood/leaf/root flux calculation behavior under positive/negative NPP.
tests/sipnet/test_modeling/Makefile	Adds new modeling test files to the test build.
tests/sipnet/test_events_types/testEventHarvest.c	Updates event test setup to use renamed carbon accounting delta field.
src/sipnet/state.h	Renames carbon accounting pool; introduces PlantSurvivalTracker; expands EventTrackers.
src/sipnet/state.c	Adds global plantSurvivalTracker and reorders globals.
src/sipnet/sipnet.c	Refactors flux calculations; adds plant survival init + mortality checks; updates trackers behavior.
src/sipnet/restart.c	Updates restart schema layout + read/write logic to include survival tracker + expanded event trackers.
src/sipnet/nitrogen.h	Renames N demand function; adds N resorption helper API; clarifies docs.
src/sipnet/nitrogen.c	Implements renamed demand function and new updateNResorptionFlux() helper.
src/sipnet/limitations.c	Updates nitrogen limitation logic to use renamed demand function.
src/sipnet/events.h	Adds PLANTDEATH event type; expands EventTrackers; fixes typo in comment.
src/sipnet/events.c	Adds plantdeath type-string support; tracks harvest fractions; extends event printing.
src/sipnet/depeffects.c	Adds missing include needed after header reshuffle.
src/sipnet/debug_log.c	Updates debug field lists and outputs to include survival tracker state.
src/sipnet/balance.c	Updates carbon accounting pool name and improves warning numeric formatting.
Makefile	Adds smokedebug target and debug-log cleanup in cleanall.
docs/CHANGELOG.md	Documents new debug viewer, plant mortality event output, and carbon accounting changes.
.github/copilot_instructions.md	Adds changelog formatting guidelines for Copilot-generated text.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 34 out of 38 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

src/sipnet/events.c:560

• A harvest can legally be encountered when the plant has no biomass (for example, after an earlier mortality), making totalMass zero. These divisions then store NaNs in the event trackers and plant-death event output; a subsequent restart checkpoint also rejects those non-finite values. Set both tracked fractions to zero when totalMass <= TINY.

        eventTrackers.harvestFracRemoved = massRemoved / totalMass;
        eventTrackers.harvestFracTransferred = massTransferred / totalMass;

src/sipnet/nitrogen.c:202

• This new demand derivation guarantees the existing testUpdateNitrogenPoolsFromStorage cases fail: those cases set only fluxes.nUptake=3, so nDemandFlux is zero and storageDemandFlux becomes -3, causing storage N to increase while mineral N decreases. Update those tests to establish creation demand and run the fixation/uptake calculation (or preserve the prior pool-update contract) so make test passes.

  double nDemandFlux = calcPlantNDemandFlux();
  double storageDemandFlux = nDemandFlux - fluxes.nUptake - fluxes.nFixation;
  // leaf-on; fluxes.eventLeafOnCreation is handled in events.c
  double leafOnNFlux = calcLeafOnNFromC(fluxes.leafOnCreation);
  envi.plantStorageN +=
      (fluxes.leafOffNResorption + fluxes.reductionNResorption -
       storageDemandFlux - leafOnNFlux) *
      climate->length;

tools/sipnet_view_common.py:535

• This shared help text is also used by sipnet-view, whose columns are unprefixed (plantWoodC, not envi.plantWoodC). Following the new example therefore makes that command reject the requested columns. Keep the shared text generic or pass viewer-specific help into this builder.
  src/sipnet/restart.c:166
• The checkpoint schema now adds schema_layout.survival_trackers_size and a survival.* payload, but the authoritative restart specification still lists only envi/trackers/phenology/event sections. Update docs/developer-guide/restart-checkpoint.md (including its layout/struct-guard lists) and the user guide as required by that document's schema-change checklist.

  state->schemaPF[ind++] = (StateField){"schema_layout.envi_size",               FT_SPECIAL, 0, RESTART_SCHEMA_LAYOUT_ENVI_SIZE};
  state->schemaPF[ind++] = (StateField){"schema_layout.trackers_size",           FT_SPECIAL, 0, RESTART_SCHEMA_LAYOUT_TRACKERS_SIZE};
  state->schemaPF[ind++] = (StateField){"schema_layout.phenology_trackers_size", FT_SPECIAL, 0, RESTART_SCHEMA_LAYOUT_PHENOLOGY_TRACKERS_SIZE};
  state->schemaPF[ind++] = (StateField){"schema_layout.survival_trackers_size",  FT_SPECIAL, 0, RESTART_SCHEMA_LAYOUT_SURVIVAL_TRACKERS_SIZE};  state->schemaPF[ind++] = (StateField){"schema_layout.event_trackers_size",     FT_SPECIAL, 0, RESTART_SCHEMA_LAYOUT_EVENT_TRACKERS_SIZE};

[dlebauer]

🥇
One inline question about N calculation logic. I will share some remaining thoughts in slack that should not hold up this PR.

[dlebauer]

It appears that permitted growth is based on current-step resorption, but soil uptake is not. The resorbed N can then remain in plantStorageN while the same N is drawn from minN. Should current resorption be included in storageFlux?

[Alomir]

@dlebauer "permitted growth" as calculated where?

I actually had it that way at first... but I spent a bit of time here thinking about the "core" of how SIPNET handles things: calculate fluxes based on current state, then update pools. Every time we calculate a flux based on a current-step flux, I feel that we should consider if we might be violating that. There are places where it's absolutely the correct thing - e.g., using C fluxes to calculate N fluxes is just inherent in our N model (though actually, we could probably do those from actual state, it would just be longer and result in the same outcome).

Here, specifically, I think we are skirting the edge of the SIPNET methodology no matter what, as this feels like a big "look ahead" to what the state is going to be at the end of the timestep. However, our desire to consider the storage N before soil N to satisfy demand necessitates some level of doing this. And, in that vein, bringing in all of the demand terms makes sense to me (and keeps the storage from going negative). However, considering what's going to come INTO the pool in this time step feels akin to, say, taking into account this step's leaf growth when calculating leaf litter (which of course we don't do).

[Alomir]

If by "permitted growth" you mean our limitation checks, then I feel like those were an exception that we (or at least I, hah hah) grudgingly accepted; I'd rather change those, than let the model drift further from the "core" I mentioned above.

But, if you mean something else, let me know! That would be something to discuss.

[dlebauer]

Thanks for clarifying, it makes sense to change the limitation checks, but I am still concerned about cases that allow minN < 0. I'll follow up in Slack.

[Alomir]

To summarize:

• Using incoming plantStorageN (from the resorption terms) as plantAvailableN is not working, since it is not available to offset minN losses directly in the same time step
  • At least, not without major shenanigans that likely violate the calcAlllFluxes->updateAllPools contract
  • Not addressing this leads to states where end the step with minN = -x and plantStorageN = +x, where x is that incoming plantStorageN
• We can still "look ahead" for the minN pool directly in calcNitrogenLimitation, but we need to stop doing the same for plantStorageN

[Alomir]

Also also, @dlebauer + chatGPT/claude did an excellent job of finding this issue

[dlebauer]

@copilot Please analyze, don’t modify. Is combining storage and mineral N here consistent with storage-first uptake in checkNitrogenLimitation()? Please test partial storage with nonzero fixation and check whether updateNitrogenPools() can leave minN < 0.

[Copilot]

It is not consistent when fixation is nonzero. I tested a partial-storage case locally (minN=0.1, plantStorageN=0.5, nFixationFracMax=0.5, halfNFixationMax=1.0, timestep 0.125, standard demand fluxes 60/500/40/100) and after calcNFixationAndUptakeFluxes() + checkLimitations() + updateNitrogenPools(), minN ended at -0.227273. So yes: in that scenario updateNitrogenPools() can leave minN < 0. I did not modify the code in this thread.

[Copilot]

@copilot please open a PR to update the unit tests in tests/sipnet to cover these changes. Create new similar tests if needed.

Addressed in b39cdcc. I added focused tests/sipnet/test_modeling/testPlantMortality.c coverage for the carbon-accounting mortality path, including cases where plantCAccountingDelta drives total wood carbon negative.

[Alomir]

@copilot please repeat this analysis, again without modifying. Is combining storage and mineral N here consistent with storage-first uptake in checkNitrogenLimitation()? Please test partial storage with nonzero fixation and check whether updateNitrogenPools() can leave minN < 0

[Copilot]

@copilot please repeat this analysis, again without modifying. Is combining storage and mineral N here consistent with storage-first uptake in checkNitrogenLimitation()? Please test partial storage wi...

I reran the analysis against the current code, without modifying anything. The current implementation is consistent with storage-first uptake:

• calcNFixationAndUptakeFluxes() first removes calcUnclaimedStorageN() from demand, then splits only the remainder between fixation and mineral uptake.
• checkNitrogenLimitation() limits that mineral-uptake portion against minN + calcMinNNonUptakeFluxes()*dt and then recomputes fixation/uplake.
• updateNitrogenPools() subtracts only nUptake from minN; the rest of the demand comes from plantStorageN.

I reran make testbuild and cd tests/sipnet/test_modeling && ./testNitrogenCycle, which now passes. For the partial-storage/nonzero-fixation case (minN=0.1, plantStorageN=0.5, nFixationFracMax=0.5, halfNFixationMax=1.0, dt=0.125), the current path gives:

• before limitation: fixation = 0.340909, uptake = 0.409091
• after limitation: fixation = 0.083333, uptake = 0.100000
• end of step: plantStorageN = 0.0, minN = 0.0

So with the current code, updateNitrogenPools() does not leave minN < 0 in that scenario.

[dlebauer]

🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳
🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳
🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳
🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳
🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳
🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳🥳