Skip to content

add fatal errors for issues with Lbins - #785

Open
iantaylor-NOAA wants to merge 1 commit into
mainfrom
Lbin-warnings
Open

iantaylor-NOAA wants to merge 1 commit into
mainfrom
Lbin-warnings

Conversation

@iantaylor-NOAA

Copy link
Copy Markdown
Contributor

Thanks @grantdadams for suggesting some additional errors.

Code changes were suggested by copilot based on imformative prompts linked to issues below and then slightly refined by me.

What tests have been done?

I modified the data.ss file in https://github.com/r4ss/r4ss/tree/main/inst/extdata/simple_small to first have Lbin_lo = 1.5 which caused the error
Warning 1 Fatal Error! Lbin_lo and Lbin_hi must be integers when Lbin_method is 1 or 2 in age comp 2011 7 1 3 0 2 1.5 1.5 25
and then to have Lbin_lo = 1 and Lbin_hi = 2 for one vector and Lbin_lo = 2 and Lbin_hi = 3 for another which produced the error
Warning 1 Fatal Error! overlapping Lbin ranges in age comps for fleet 1: 2-3 and 1-2

What tests/review still need to be done?

Is there an input change for users to Stock Synthesis?

Additional information (optional).

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

Restrict overlap checks to matching observation contexts and avoid the costly global pairwise scan.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Adds Lbin validation and terminology updates for age-composition data.

Changes:

  • Rejects fractional Lbin values for methods 1 and 2.
  • Detects overlapping Lbin ranges.
  • Standardizes L_bin terminology to Lbin.
File Summary
SS_readdata_330.tpl Adds Lbin validation and overlap detection; the overlap check may reject valid rows across contexts and has potentially quadratic runtime.

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

Comment thread SS_readdata_330.tpl
Comment on lines +2678 to +2681
for (int j1 = 1; j1 < j; j1++)
{
if ((Lbin_hi(f, j) > Lbin_lo(f, j) && Lbin_hi(f, j) == Lbin_lo(f, j1)) ||
(Lbin_hi(f, j1) > Lbin_lo(f, j1) && Lbin_hi(f, j1) == Lbin_lo(f, j)))

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.

I agree with the copilot review. I will look more closely tomorrow to see how it might be addressed.

@grantdadams

Copy link
Copy Markdown

Thanks @iantaylor-NOAA! Love a good edge case trap!

@Rick-Methot-NOAA

Copy link
Copy Markdown
Collaborator

Thanks Grant for a great description of the issue and thanks Ian, with his trusty copilot, CoPilot, for the fix.
I have no recollection regarding why this overlap test was done with cross-observation checks, but no need now to revisit that.

@iantaylor-NOAA

iantaylor-NOAA commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor Author

@Rick-Methot-NOAA, to clarify, the overlap test was added by me thinking this would avoid the situation that Grant described in nmfs-ost/ss3-doc#339 (comment):

  1. Method 3's Lbin_hi convention deserves a sentence. §7.16.3.2 says method 3 values "must
    correspond to population length bin boundary", but not that Lbin_hi is the lower edge of the
    last bin included rather than the top of the interval. A 5 cm bin at 9.5 cm on a
    1 cm population grid is 9.5 13.5, not 9.5 14.5.

The copilot review (of the copilot-generated code) is right that the current test is probably too broad, although it's hard to imagine someone intentionally using different length bin ranges for different fleets.

@Rick-Methot-NOAA

Copy link
Copy Markdown
Collaborator
  1. The original intent of the code was to provide alternative ways to tell the model which range of poplen_bins to use when processing a CAAL observation. There is no associated code to provide interpolation capability and the comments in the code all refer to finding matches for the Lbin_lo and lbin_hi inputs.
  2. poplen_bins can, in principal, be non-integer. They are in the krill model I am currently advising on. So the check for non-integer values can fail.
  3. We need to make in clear in the documentation that both Lbin_lo and Lbin_hi point to the lower edge of poplen_bins.
  4. Perhaps the easiest failsafe is to check each entry of a Lbin_lo or lbin_hi value against the list of poplen_bin values. This is exactly what happens for method 2. Do we already have a check regarding data lenbins aligning with poplenbins?

@iantaylor-NOAA

Copy link
Copy Markdown
Contributor Author

@Rick-Methot-NOAA, thanks for looking into this further.

The new check for integer values (starting in this line https://github.com/nmfs-ost/ss3-source-code/pull/785/changes#diff-c9ec45f140c8bc97f6c2112285577e4c1f538c6d1d60814bc854370ea0a70395R2592) should only be applied when Lbin_method is 1 or 2, so decimal values for the bin boundaries entered associated with Lbin_method = 3 should not trigger the error.

I can look into what other warnings occur if there's a mismatch, but might not be able to get to it until next week.

Maybe it makes sense to update the user manual before finalizing this PR so we can make sure the warnings or errors are consistent with the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: non-integer Lbin_lo/Lbin_hi are silently truncated under Lbin_method = 1, while method 3 raises a fatal error for the equivalent mistake

4 participants