Skip to content

Address PR #248 review feedback: document wood carbon storage pool#253

Merged
dlebauer merged 38 commits into
masterfrom
copilot/sub-pr-248-again
Feb 16, 2026
Merged

Address PR #248 review feedback: document wood carbon storage pool#253
dlebauer merged 38 commits into
masterfrom
copilot/sub-pr-248-again

Conversation

Copilot AI commented Feb 6, 2026

Copy link
Copy Markdown
Contributor
  • Revert code changes (balance.c and state.h) per reviewer request
  • Remove Mass Balance Checks section from model-structure.md (not required)
  • Update Wood Carbon section in model-structure.md to describe explicit storage pool
  • Add plantWoodCStorageDelta documentation to parameters.md
  • Remove accidentally committed test output files
  • Update .gitignore to exclude generated test outputs
  • Fix line 137 in model-structure.md: remove "accurate" and code parameter name
  • Run tests - all pass

Summary

Per review feedback, this PR now focuses only on documentation updates:

Documentation Changes

model-structure.md:

  • Updated Wood Carbon section to describe that the storage pool is now explicitly tracked as $C_{\text{wood,storage}}$ (starting in v2.1)
  • Added equation for total wood carbon: $C_{\text{wood,total}} = C_{\text{wood}} + C_{\text{wood,storage}}$
  • Explained storage represents lag between NPP input and allocation output due to 5-day averaging
  • Removed "accurate" qualifier (implied)
  • Removed code parameter name from documentation (kept only mathematical notation)

parameters.md:

  • Added entry for $C_{\text{wood,storage}}$ (plantWoodCStorageDelta) in state variables table
  • Documented that it tracks NPP/allocation lag, is initialized to 0, and can be negative
  • Clarified total wood C is sum of structural wood + storage

CHANGELOG.md:

Code Changes

  • All code changes reverted per reviewer request (balance.c and state.h restored to original state)

Test Files

  • Removed accidentally committed test output files (events.out)
  • Updated .gitignore to properly exclude generated test outputs while keeping expected outputs

All tests pass.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits February 6, 2026 03:30
Co-authored-by: dlebauer <464871+dlebauer@users.noreply.github.com>
Co-authored-by: dlebauer <464871+dlebauer@users.noreply.github.com>
…r review

Co-authored-by: dlebauer <464871+dlebauer@users.noreply.github.com>

Copilot AI commented Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

Needs to include description of the storage pool.

Needs to update parameters. ...

Completed in 1a071ee:

Storage pool description added: Updated Wood Carbon section in model-structure.md to document plantWoodCStorageDelta as explicit storage pool tracking NPP/allocation lag
Parameters updated: Added entry in parameters.md for $C_{\text{wood,storage}}$ with full description
Code reverted: All code changes in balance.c and state.h have been reverted to original state

The documentation now properly describes the explicit storage pool implementation without making code changes.

Copilot AI changed the title Address PR #248 review feedback: fix typos, clarify comments, and add documentation Address PR #248 review feedback: document wood carbon storage pool Feb 6, 2026
Copilot AI requested a review from dlebauer February 6, 2026 05:45

@dlebauer dlebauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are doc updates accompanied by updates to test files?

Comment thread docs/model-structure.md Outdated
We can represent this storage of carbon conceptually as:
where it acts as a storage pool, and all allocations from the averaged NPP are deducted from that pool.

Starting in SIPNET v2.1, to support accurate mass balance tracking, this storage is explicitly tracked as a separate pool called $C_{\text{wood,storage}}$ (`plantWoodCStorageDelta` in code). We can represent this storage of carbon as:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "accurate". It is implied. Don't add parameter name used in code here, that isn't done elsewhere in this file. The mapping between mathematical symbols and parameter names are in parameters.md. Does the document have sufficient description of the dynamics added in the PR that this PR will be merged into? If needed, make sure it is consistent with the document as a whole and the current state of the code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 305769d:

  • Removed "accurate" qualifier
  • Removed code parameter name (plantWoodCStorageDelta)
  • Line now reads: "Starting in SIPNET v2.1, to support mass balance tracking, this storage is explicitly tracked as a separate pool called $C_{\text{wood,storage}}$."

The mathematical notation $C_{\text{wood,storage}}$ is mapped to the code name in parameters.md as requested.

Co-authored-by: dlebauer <464871+dlebauer@users.noreply.github.com>

Copilot AI commented Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

Why are doc updates accompanied by updates to test files?

The test output files (events.out) were accidentally committed when tests were run during development. Removed in 305769d. Also updated .gitignore to properly exclude generated test outputs (events.out, sipnet.out, sipnet.config) while preserving expected output files.

Copilot AI requested a review from dlebauer February 6, 2026 06:14
Base automatically changed from add_carbon_balance_check to master February 6, 2026 17:02
@Alomir Alomir marked this pull request as ready for review February 6, 2026 17:11
@Alomir

Alomir commented Feb 9, 2026

Copy link
Copy Markdown
Collaborator

I updated C_wood,storage description and a few other spots, but also accidentally re-wrapped the model-structure.md file 😬

@dlebauer, please review

@dlebauer dlebauer merged commit db925d1 into master Feb 16, 2026
11 checks passed
@dlebauer dlebauer deleted the copilot/sub-pr-248-again branch February 16, 2026 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants