[339] Rename plantWoodCStorageDelta to plantWoodCAccountingDelta#344
Open
ANAMASGARD wants to merge 2 commits into
Open
[339] Rename plantWoodCStorageDelta to plantWoodCAccountingDelta#344ANAMASGARD wants to merge 2 commits into
ANAMASGARD wants to merge 2 commits into
Conversation
The wood N-lag term is an accounting delta for carbon not coupled to nitrogen, not a storage pool. Renames the Envi field, sipnet.out column (was nppStorage), restart key, and docs; legacy restart keys remain readable.
ANAMASGARD
requested review from
Alomir,
dlebauer,
mdietze and
mswilburn
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR renames SIPNET’s internal “wood storage delta” field to clarify that it is an accounting term (carbon not coupled to nitrogen), and propagates the rename through outputs, restart serialization, tests, and docs.
Changes:
- Renamed
plantWoodCStorageDelta→plantWoodCAccountingDeltaacross model code and tests. - Renamed the
sipnet.outcolumnnppStorage→plantWoodCAccountingDeltaand updated smoke-check tooling and golden outputs. - Added backward-compatible restart reading for legacy checkpoints using
envi.plantWoodCStorageDelta.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/smoke_check.py | Updates expected output column list to match the renamed output field. |
| tests/smoke/russell_4/sipnet.out | Updates smoke-test golden output header for the renamed column. |
| tests/sipnet/test_restart_infrastructure/mock_state.c | Updates restart infra test scaffolding for the renamed environment field. |
| tests/sipnet/test_events_types/testEventHarvest.c | Updates event test initialization to the renamed environment field. |
| src/sipnet/state.h | Renames the environment struct field and clarifies its meaning in comments. |
| src/sipnet/sipnet.c | Renames output column header and replaces all uses of the old field name. |
| src/sipnet/restart.c | Writes restart checkpoints with the new key and reads legacy checkpoints with the old key. |
| src/sipnet/events.c | Updates harvest event calculations to use the renamed field. |
| src/sipnet/balance.c | Updates mass-balance totals and clarifies nitrogen accounting comment. |
| docs/user-guide/model-outputs.md | Documents the new output column name and the prior name. |
| docs/parameters.md | Updates parameter documentation to the new accounting-delta terminology. |
| docs/model-structure.md | Updates model-structure narrative/equations to reflect accounting delta terminology. |
| docs/developer-guide/restart-checkpoint.md | Documents backward-compatible restart key handling post-rename. |
| docs/CHANGELOG.md | Adds changelog entry describing the rename and restart compatibility behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #define RESTART_SCHEMA_VERSION "1.0" | ||
| #define RESTART_FLOAT_EPSILON 1e-8 | ||
| #define LEGACY_ENVI_PLANT_WOOD_C_STORAGE_DELTA "envi.plantWoodCStorageDelta" | ||
| #define ENVI_PLANT_WOOD_C_ACCOUNTING_DELTA_INDEX 12 |
| state->enviPF[10] = (StateField){"envi.litterN", FT_DOUBLE, &envi.litterN, 0}; | ||
| state->enviPF[11] = (StateField){"envi.plantStorageN", FT_DOUBLE, &envi.plantStorageN, 0}; | ||
| state->enviPF[12] = (StateField){"envi.plantWoodCStorageDelta", FT_DOUBLE, &envi.plantWoodCStorageDelta, 0}; | ||
| state->enviPF[12] = (StateField){"envi.plantWoodCAccountingDelta", FT_DOUBLE, &envi.plantWoodCAccountingDelta, 0}; |
Member
There was a problem hiding this comment.
I don't think we want to support the legacy key
Comment on lines
+629
to
+631
| if (setLegacyPlantWoodCAccountingDelta( | ||
| restartIn, key, value, | ||
| &state->enviPF[ENVI_PLANT_WOOD_C_ACCOUNTING_DELTA_INDEX])) { |
Member
There was a problem hiding this comment.
I don't think we want to support the legacy key
|
|
||
| fprintf(out, "%4d %3d %5.2f %10.2f %10.2f %12.2f ", year, day, time, | ||
| (envi.plantWoodC + envi.plantWoodCStorageDelta), envi.plantLeafC, | ||
| (envi.plantWoodC + envi.plantWoodCAccountingDelta), envi.plantLeafC, |
dlebauer
reviewed
May 27, 2026
Member
dlebauer
left a comment
dlebauer
left a comment
There was a problem hiding this comment.
@ANAMASGARD - thank you for your contribution!
I made a few comments, but will defer to @Alomir to determine what, if anything, should be changed before merging.
| state->enviPF[10] = (StateField){"envi.litterN", FT_DOUBLE, &envi.litterN, 0}; | ||
| state->enviPF[11] = (StateField){"envi.plantStorageN", FT_DOUBLE, &envi.plantStorageN, 0}; | ||
| state->enviPF[12] = (StateField){"envi.plantWoodCStorageDelta", FT_DOUBLE, &envi.plantWoodCStorageDelta, 0}; | ||
| state->enviPF[12] = (StateField){"envi.plantWoodCAccountingDelta", FT_DOUBLE, &envi.plantWoodCAccountingDelta, 0}; |
Member
There was a problem hiding this comment.
I don't think we want to support the legacy key
Comment on lines
+629
to
+631
| if (setLegacyPlantWoodCAccountingDelta( | ||
| restartIn, key, value, | ||
| &state->enviPF[ENVI_PLANT_WOOD_C_ACCOUNTING_DELTA_INDEX])) { |
Member
There was a problem hiding this comment.
I don't think we want to support the legacy key
Per review on PecanProject#344: do not read envi.plantWoodCStorageDelta from checkpoints. Update docs and add test that the obsolete key is rejected.
Contributor
Author
|
Thanks @dlebauer for the review — addressed in this new commit
Left the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FIXES #339.
Issue #339 refers to
plantWoodStorageC; the actual SIPNET identifier wasplantWoodCStorageDelta(output column:nppStorage). This PR renames it toplantWoodCAccountingDeltato reflect that it tracks carbon changes with no associated nitrogen (NPP averaging lag / bookkeeping), not a storage pool.sipnet.out: columnnppStorage→plantWoodCAccountingDelta(wider field width for header alignment).envi.plantWoodCAccountingDelta; existing checkpoints usingenvi.plantWoodCStorageDeltastill load.Test plan
make testbuildmake unit(all 21 tests pass, includingtestOutputHeaderand restart suite)make smoke(5/5 sipnet; smoke baselines updated for last-column padding)