Skip to content

[278] Require positive allocations and reject nonfinite parameters - #405

Merged
Alomir merged 5 commits into
PecanProject:masterfrom
Nitir4:278-reject-negative-allocations
Nov 4, 2026
Merged

Alomir merged 5 commits into
PecanProject:masterfrom
Nitir4:278-reject-negative-allocations

Conversation

@Nitir4

@Nitir4 Nitir4 commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Require positive NPP allocation fractions for leaves, wood, fine roots, and the derived coarse-root remainder. The three supplied fractions must each be below one and sum to less than one. Zero, negative, and nonfinite allocation inputs exit with code 3.

The parameter-file reader also rejects NaN, infinity, and overflow to infinity in supplied, recognized model parameters before assignment. Omitted optional parameters and ignored legacy names retain their existing behavior.

The strict-positive allocation rule follows Alomir's review. This addresses part of #278; parameter-specific ranges, climate and event validation remain outside this PR.

How was this change tested?

  • make -B test: 27 test executables and four active smoke scenarios passed. russell_4 remains a forced skip.
  • make testrun: passed.
  • Allocation regression: 24 cases, including zero, signed zero, an exactly-one total, valid small-positive allocations, and existing negative/upper-bound cases. Eight cases failed before the strict-positive change and passed afterward.
  • Existing nonfinite regression: 18 reader cases and nine direct allocation cases passed.
  • Fifteen parameter-file CLI cases passed with the expected exit codes.
  • clang-format 19.1.7 and git diff --check passed.

No smoke reference outputs were changed. Local clang-tidy and documentation-site builds were not run.

Reproduction steps

Set any supplied allocation to zero, or use (0.25, 0.25, 0.5) so the coarse-root remainder is zero. Model setup exits with code 3. (0.25, 0.25, 0.25) remains valid. A recognized parameter set to nan or inf also exits with code 3.

Related issues

Addresses part of #278. The allocation rule was updated in response to the review on this PR.

Checklist

Copilot AI balanced review requested due to automatic review settings October 12, 2026 10:59

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 was unable to review this pull request because the user who requested the review has reached their quota limit.

@Alomir Alomir left a comment

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.

Nice work, thank you for your contribution @Nitir4!

I have a request - would you please also enforce that all four allocation parameters are greater than zero?

That's a change from allowing them to be zero, which was what the issue originally asked. However, after discussion with @dlebauer, we've decided it makes more sense for the allocation params to be greater than zero.

Comment thread docs/CHANGELOG.md Outdated

### Fixed

- Reject negative leaf, wood, and fine-root NPP allocation parameters during model setup (#278).

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.

We put the PR number instead of the tracking number into CHANGELOG - so this should be #405

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.

Thanks for reviewing. I’ll make all four allocations strictly positive, update the tests and docs, and change the changelog references to #405.

@Nitir4 Nitir4 changed the title [278] Reject negative NPP allocation inputs [278] Require positive allocations and reject nonfinite parameters Oct 28, 2026
@infotroph

Copy link
Copy Markdown
Member

makes more sense for the allocation params to be greater than zero

@Alomir Can you fill me in on the logic here? If we're simulating something fully nonwoody I'm not sure why we'd need to keep adding epsilon to the wood pool.

@dlebauer

Copy link
Copy Markdown
Member

@infotroph: in non woody plants the woody represents structural / non-photosynthetic and storage.

@Nitir4
Nitir4 requested a review from Alomir October 30, 2026 09:04

@Alomir Alomir left a comment

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.

Great, thanks for the changes and your contribution overall!

@Alomir
Alomir merged commit b16fb96 into PecanProject:master Nov 4, 2026
12 checks passed
@Nitir4

Nitir4 commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @Alomir for the review and guidance, and thanks for merging! Looking forward to contributing more.

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.

5 participants