Skip to content

[185] Update N cycle documentation#342

Merged
dlebauer merged 4 commits into
PecanProject:masterfrom
Sanketmandwal:185-update-n-cycle-docs
Jun 7, 2026
Merged

[185] Update N cycle documentation#342
dlebauer merged 4 commits into
PecanProject:masterfrom
Sanketmandwal:185-update-n-cycle-docs

Conversation

@Sanketmandwal

Copy link
Copy Markdown
Contributor

Summary

  • What: Updated nitrogen cycle documentation in docs/model-structure.md and docs/parameters.md to reflect the implemented N-cycle behavior.

    Changes include:

    • Added K_vol to Nitrogen Cycle Parameters
    • Removed redundant documentation related to fN2O_vol
    • Added fNLeach
    • Updated nitrogen volatilization documentation and moved justification below the model description/equation
    • Updated nitrogen leaching equation to:
      F^N_leach = N_min · φ · fN_leach
    • Added:
      φ = min(F^W_drainage / W_WHC, 1)
    • Updated documentation to reflect a single minN pool while preserving separate soil and litter mineralization fluxes
    • Replaced future tense with present tense where appropriate
  • Motivation: The documentation did not fully match the implemented nitrogen cycle behavior and parameter definitions. This update aligns the documentation with the current implementation and removes outdated or redundant descriptions.

How was this change tested?

  • Previewed documentation locally using:
mkdocs serve
  • Verified documentation builds successfully.

  • Manually reviewed rendered pages for:

    • Nitrogen Volatilization section
    • Nitrogen Leaching section
    • Nitrogen Cycle Parameters table
  • Reviewed git diff to confirm changes are limited to intended documentation updates.

No smoke tests or code execution changes were required since this PR only updates documentation.

Reproduction steps

  1. Run:
mkdocs serve
  1. Open:
http://127.0.0.1:8000/
  1. Navigate to the updated nitrogen cycle documentation sections.

Related issues

Checklist

  • Related issues are listed above.
  • PR title has the issue number in it
  • Tests added/updated for new features (if applicable)
  • Documentation updated (if applicable)
  • docs/CHANGELOG.md updated with noteworthy changes
  • Code formatted with clang-format (not applicable for documentation-only changes)

Copilot AI review requested due to automatic review settings May 27, 2026 07:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates nitrogen-cycle documentation to reflect a single mineral nitrogen pool and clarifies volatilization/leaching parameter definitions and equations.

Changes:

  • Renames initial mineral N state symbol to a single shared $N_{\text{min},0}$ pool.
  • Rewrites nitrogen volatilization and leaching documentation (including justification and updated leaching scaling).
  • Adjusts temperature-response equation formatting and clarifies moisture-fraction wording.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
docs/parameters.md Updates parameter table entries and nitrogen-cycle parameter definitions/units to match revised model description.
docs/model-structure.md Updates nitrogen volatilization/leaching sections to reflect single $N_\text{min}$ pool and introduces $\phi$ scaling; minor equation formatting edits.

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

Comment thread docs/parameters.md Outdated
Comment on lines +205 to +208
| Symbol | Parameter Name | Definition | Units | Notes |
| ---------------------- | -------------------- | ------------------------------------------------------------------------------------------------------------------------------------ | ----------------- | -------------------------------- |
| $K_\text{vol}$ | nVolatilizationFrac | Nitrogen volatilization rate constant that determines the maximum rate of N volatilization as a proportion of available $N_\text{min}$ | $\text{day}^{-1}$ | \eqref{eq:n_vol} |
| $f^N_{\text{leach}}$ | nLeachingFrac | Fraction of $N_\text{min}$ available to be leached, applied after scaling by $\phi = \min(F^W_\text{drainage}/W_\text{WHC}, 1)$ | unitless | \eqref{eq:n_leach} |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, nLeachingFrac should have units of day^-1 (per day); this should be updated here and in state.h

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.

Updated the units in state.h to match the documentation (per day)

Comment thread docs/model-structure.md Outdated

\begin{equation}
F^N_\text{leach} = N_\text{min} \cdot F^W_{drainage} \cdot f_{N leach}
F^N_\text{leach} = N_\text{min} \cdot \phi \cdot f_{N leach}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

f^N_\text{leach} should be used everywhere.

Comment thread docs/model-structure.md
Comment on lines +599 to +605
where:

\begin{equation}
\phi = \min\left(\frac{F^W_\text{drainage}}{W_\text{WHC}}, 1\right)
\end{equation}

$f^N_\text{leach}$ is the fraction of $N_\text{min}$ available to be leached, $F^W_\text{drainage}$ is drainage, and

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same as above

Comment thread docs/model-structure.md Outdated
mineral nitrogen pool. The realized volatilization flux is proportional to $N_\text{min}$ and depends on temperature and
soil moisture.
$K_\text{vol}$ is the nitrogen volatilization rate constant that determines the maximum rate of N volatilization as a
proportion of available $N_\text{min}$. The realized volatilization flux is proportional to available Nmin through Kvol and depends on temperature and soil moisture.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Agreed with copilot's recommendation

Comment thread docs/model-structure.md Outdated
Comment on lines +915 to +917
\[
D_{\text{temp,Q10}} = Q_{10}^{\frac{(T-T_\text{opt})}{10}}
\label{eq:Braswell_A18b}
\end{equation}
\]
@Sanketmandwal

Copy link
Copy Markdown
Contributor Author

Hello @dlebauer sir , I have made changes. Can you please review it once ? Tell me if any correction is there.

@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.

Thanks for these changes - sorry I reviewed and had suggestions but forgot to submit them. Please see comments in line.

Comment thread docs/model-structure.md Outdated
Moisture dependence functions are typically based on soil water content as a fraction of water holding capacity, also
referred to as soil moisture or fractional soil wetness. We will represent this fraction of soil wetness
as $f_\text{WHC}$.
referred to as soil moisture or fractional soil wetness. SIPNET represents this fraction of soil wetness 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.

Suggested change
referred to as soil moisture or fractional soil wetness. SIPNET represents this fraction of soil wetness as
referred to as soil moisture or fractional soil wetness

Comment thread docs/model-structure.md Outdated
referred to as soil moisture or fractional soil wetness. We will represent this fraction of soil wetness
as $f_\text{WHC}$.
referred to as soil moisture or fractional soil wetness. SIPNET represents this fraction of soil wetness as
$f_\text{WHC}$.

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.

Suggested change
$f_\text{WHC}$.
($f_\text{WHC}$).

Comment thread docs/parameters.md Outdated
| $N_{\text{org, litter},0}$ | litterOrgNInit | Initial litter organic nitrogen content | $\text{g N} \cdot \text{m}^{-2}$ | |
| $N_{\text{org, soil},0}$ | soilOrgNInit | Initial soil organic nitrogen content | $\text{g N} \cdot \text{m}^{-2}$ | |
| $N_{\text{min, soil},0}$ | mineralNInit | Initial soil mineral nitrogen content | $\text{g N} \cdot \text{m}^{-2}$ | |
| $N_{\text{min},0}$ | mineralNInit | Initial mineral nitrogen content | $\text{g N} \cdot \text{m}^{-2}$ | Single mineral N pool used by soil and litter N fluxes |

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.

Although this is a simplifying abstraction, even though both litter and soil contribute to this pool, it still functions as a soil mineral N pool because it is available to roots.

I'd prefer to keep the soil subscript, and use the description to clarify.

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.

Updated as suggested, thanks

Comment thread docs/parameters.md
Comment on lines +207 to +208
| $K_\text{vol}$ | nVolatilizationFrac | Nitrogen volatilization rate constant that determines the maximum rate of N volatilization as a proportion of available $N_\text{min}$ | $\text{day}^{-1}$ | \eqref{eq:n_vol} |
| $f^N_{\text{leach}}$ | nLeachingFrac | Fraction of $N_\text{min}$ available to be leached, applied after scaling by $\phi = \min(F^W_\text{drainage}/W_\text{WHC}, 1)$ | $\text{day}^{-1}$ | \eqref{eq:n_leach} |

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.

Should these both be f?

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.

I kept $K_\text{vol}$ because SIP89 and issue #185 use K_vol for the volatilization rate constant, and it matches the implemented nVolatilizationFrac parameter and existing model equations. I used $f^N_\text{leach}$ for leaching to stay consistent with the current leaching notation and recent review comments. If you'd prefer both parameters to follow the same notation convention, I'm happy to update them consistently.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread docs/parameters.md
| Symbol | Parameter Name | Definition | Units | Notes |
| ---------------------- | -------------------- | ------------------------------------------------------------------------------------------------------------------------------------ | ----------------- | -------------------------------- |
| $K_\text{vol}$ | nVolatilizationFrac | Nitrogen volatilization rate constant that determines the maximum rate of N volatilization as a proportion of available $N_\text{min}$ | $\text{day}^{-1}$ | \eqref{eq:n_vol} |
| $f^N_{\text{leach}}$ | nLeachingFrac | Fraction of $N_\text{min}$ available to be leached, applied after scaling by $\phi = \min(F^W_\text{drainage}/W_\text{WHC}, 1)$ | $\text{day}^{-1}$ | \eqref{eq:n_leach} |
Comment thread docs/model-structure.md
Comment on lines +601 to +603
\begin{equation}
\phi = \min\left(\frac{F^W_\text{drainage}}{W_\text{WHC}}, 1\right)
\end{equation}
@dlebauer dlebauer dismissed their stale review June 6, 2026 21:39

Addressed

@dlebauer dlebauer enabled auto-merge (squash) June 6, 2026 21:40
@dlebauer

dlebauer commented Jun 6, 2026

Copy link
Copy Markdown
Member

Thanks!

@dlebauer dlebauer merged commit 7b2a515 into PecanProject:master Jun 7, 2026
12 of 13 checks passed
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.

Update documentation for N cycle

4 participants