[iantaylor-NOAA]

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.

• fixes [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 #784 by adding a fatal error for non-integer Lbin values when Lbin_method is 1 or 2
• also addresses something in CAAL example in §7.17 does not say which bin method it assumes, and the manual never states that Lbin_lo/Lbin_hi must be integers under methods 1 and 2 ss3-doc#339 by adding a fatal error if the Lbin_hi > Lbin_lo in one vector (so conditioned on multiple length bins) AND the Lbin_hi in that vector matches the Lbin_lo of a different vector. @Rick-Methot-NOAA, can you think of a case where a user would want to have overlapping length bins? If so, we could shift this second error to just a warning.
• aligns terminology by changing a few instances of "L_bin" to "Lbin"

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]

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

Open (1)

• Restrict age-bin overlap checks to matching observation contexts · New

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.

[grantdadams]

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

[Rick-Methot-NOAA]

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]

@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):

4. 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]

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]

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

[iantaylor-NOAA]

The failed tests for this PR might be caused by the same bug in the comparison script that @e-perl-NOAA fixed as discussed in #783 (comment). Therefore, I would suggest merging #783 first, rebasing this branch onto the corrected main, and then re-running the run-* github actions on this branch to confirm that it's OK. Or we could rebase onto new_general_sizebin_method to do the test before merging.

[e-perl-NOAA]

I've rebased onto main so if the tests pass then it should be good to go!

[e-perl-NOAA]

The failed models are the same ones that were failing in the PR that Rick just merged, these workflows won't pass until the test models repo is updated with this next release