add fatal errors for issues with Lbins - #785
iantaylor-NOAA wants to merge 1 commit into
Conversation
- fixes #784 - related to nmfs-ost/ss3-doc#339
There was a problem hiding this comment.
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
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_binterminology toLbin.
| 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.
| 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))) |
There was a problem hiding this comment.
I agree with the copilot review. I will look more closely tomorrow to see how it might be addressed.
|
Thanks @iantaylor-NOAA! Love a good edge case trap! |
|
Thanks Grant for a great description of the issue and thanks Ian, with his trusty copilot, CoPilot, for the fix. |
|
@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):
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, 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. |

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.
Lbin_lo/Lbin_hiare silently truncated underLbin_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 2Lbin_lo/Lbin_himust 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.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 25and 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-2What tests/review still need to be done?
Is there an input change for users to Stock Synthesis?
Additional information (optional).