Skip to content

Convert negative stock warning to error - #398

Merged
dlebauer merged 4 commits into
masterfrom
SIP365-Have-ensureNonNegative-error-instead-of-warn
Oct 2, 2026
Merged

Convert negative stock warning to error#398
dlebauer merged 4 commits into
masterfrom
SIP365-Have-ensureNonNegative-error-instead-of-warn

Conversation

@Alomir

@Alomir Alomir commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

  • What: Convert the warning in ensureNonNegative to an error
  • Motivation: Too many cases of (silently!) treating bad output as good

How was this change tested?

All unit and smoke tests pass.

Related issues

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)

@Alomir
Alomir marked this pull request as ready for review September 25, 2026 18:00
Copilot AI lite review requested due to automatic review settings September 25, 2026 18:00
Copilot AI reviewed Sep 25, 2026
View reviewed changes

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.

Copilot review overview

🟡 Changes recommended

The implementation can terminate on valid small positive snow values, and documentation updates are still needed.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 2 Low severity

Open (3)
What changed in this PR

This PR changes negative stock handling from warnings to fatal errors and documents the behavior change.

Changes:

  • Updates ensureNonNegative error handling.
  • Adds an Unreleased changelog entry.
File Summary
src/​sipnet/​sipnet.c Changes negative stock validation behavior.
docs/​CHANGELOG.md Documents the behavior change.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/sipnet/sipnet.c Outdated
Comment on lines +1352 to +1355
logError("Non-negative stock constraint applied for %s (value %8.5f set "
"to zero) year %d day %d time %6.3f\n",
label, *var, climate->year, climate->day, climate->time);
exit(EXIT_CODE_INTERNAL_ERROR);

@Alomir Alomir Sep 29, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ah, the snow case... good catch copilot!

Comment thread docs/CHANGELOG.md Outdated
Comment thread src/sipnet/sipnet.c Outdated
Comment on lines +1352 to +1355
logError("Non-negative stock constraint applied for %s (value %8.5f set "
"to zero) year %d day %d time %6.3f\n",
label, *var, climate->year, climate->day, climate->time);
exit(EXIT_CODE_INTERNAL_ERROR);
dlebauer
dlebauer reviewed Sep 29, 2026
View reviewed changes

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

This looks good to me. deferring approval until Copilot comments about aborting on valid positive snow values is addressed.

Alomir and others added 2 commits September 29, 2026 12:30
@Alomir
Fix typo
eb56ac0 
Updated changelog to reflect error behavior change for negative environment pools.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@Alomir
Updates from feedback in #398
a1d45a9
@Alomir

Alomir commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

@dlebauer - I believe this is good to go

@dlebauer
dlebauer merged commit 0426360 into master Oct 2, 2026
12 checks passed
@dlebauer
dlebauer deleted the SIP365-Have-ensureNonNegative-error-instead-of-warn branch October 2, 2026 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@dlebauer dlebauer dlebauer approved these changes
@mdietze mdietze Awaiting requested review from mdietze mdietze is a code owner
@mswilburn mswilburn Awaiting requested review from mswilburn mswilburn is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Have ensureNonNegative() error instead of warn

3 participants

@Alomir @dlebauer