Repository navigation
[#411] Make carbon and nitrogen balance failures fatal - #414
Merged
Alomir merged 2 commits intoOct 28, 2026
Merged
Conversation
priyanshusky0
requested review from
Alomir,
dlebauer,
mdietze and
mswilburn
as code owners
October 21, 2026 08:36
This was referenced Oct 21, 2026
Alomir
requested changes
Oct 27, 2026
Alomir
left a comment
Collaborator
There was a problem hiding this comment.
Looks good, thanks for contributing! One minor change suggested.
|
|
||
| ### Changed | ||
| - SIPNET will now error instead of warning when an environment pool goes negative. This is a change from previous behavior where SIPNET would log a warning and continue running. (#398) | ||
| - SIPNET now exits with `EXIT_CODE_INTERNAL_ERROR` when a timestep fails the carbon or nitrogen balance check (residual magnitude >= 1e-8, or a NaN/infinite residual), instead of logging a warning and continuing. The error reports the residual, timestep, pool totals, inputs, outputs, and clamping adjustment at full precision. (#411) |
Collaborator
There was a problem hiding this comment.
This is too much detail. How about just:
Suggested change
| - SIPNET now exits with `EXIT_CODE_INTERNAL_ERROR` when a timestep fails the carbon or nitrogen balance check (residual magnitude >= 1e-8, or a NaN/infinite residual), instead of logging a warning and continuing. The error reports the residual, timestep, pool totals, inputs, outputs, and clamping adjustment at full precision. (#411) | |
| - SIPNET now errors and exits when a mass balance check failure occurs, instead of logging a warning and continuing (#414) |
Note also that we have been putting the PR numbers in CHANGELOG instead of the issue numbers
checkBalance() now exits with EXIT_CODE_INTERNAL_ERROR when either residual is NaN/inf or has magnitude >= EPS, instead of only logging a warning. Finite residuals below EPS are still treated as noise. The error message now includes the residual, timestep, pool totals, inputs, outputs and clamping adjustment, printed with %.17g so values near EPS are visible. Added testBalanceCheck covering positive/negative imbalances, values just below and at EPS, and NaN/inf for both carbon and nitrogen.
5 of 6 tasks
priyanshusky0
force-pushed
the
411-fatal-balance-check
branch
from
October 28, 2026 05:35
b0cd100 to
724b95e
Compare
5 of 6 tasks
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
checkBalance()now exits withEXIT_CODE_INTERNAL_ERRORwhen the C or N balance residual is >= EPS (1e-8) in magnitude, or is NaN/inf. Before it only logged a warning and kept going.fabs(NaN) < EPSandfabs(NaN) > 0are false.Residuals below EPS are still zeroed and treated as noise, same as before. I didn't touch the balance equations or the clamping accounting.
On failure it now logs the element, residual, year/day/time, pre/post-update/final pool totals, inputs, outputs and the clamped amount. I used
%.17gfor these so you can actually see a ~1e-8 difference when the pool totals are large. If both C and N fail in the same step, both get reported before exiting.How was this change tested?
tests/sipnet/test_modeling/testBalanceCheck.c. It sets up the balance tracker directly and checks, for both C and N: zero, +/- 0.5*EPS and the value just below EPS pass; +/- EPS and +/- 1e-3 exit; NaN, +inf and -inf exit. There's also a balanced case with a nonzero clamping adjustment.>=to>and removed theisfinitecheck in the source; the test failed in both cases.make unitpasses.make smokepasses, with no changes to anysipnet.out. I also ran each smoke case with quiet off on master first, and none of them were hitting the balance warning, so nothing new should start failing. russell_4 has askipfile so I ran it by hand too, it exits 0.Related issues
Checklist
docs/CHANGELOG.mdupdated with noteworthy changesclang-format(rungit clang-formatif needed)