revise code and change VBK_seas usage #747
Conversation
|
I will test with big skate locally to diagnose the run failure |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the growth calculation code in Stock Synthesis, focusing on improving variable naming clarity and revising how seasonal growth adjustments (VBK_seas) are applied. The changes address issues #735 and #156.
Key changes:
- Renamed growth-related variables for clarity (VBK_temp → VBK_work, VBK_temp2 → VBK_by_seas)
- Restructured growth type handling with improved switch statement organization
- Modified VBK_seas usage by removing seasdur(s) multipliers in growth calculations
- Added documentation comments explaining VBK variable purposes
- Removed preprocessor conditionals (#ifdef DO_ONCE) in favor of runtime checks
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| VBK_temp2 = (mfexp(VBK_temp * seasdur(s) * VBK_seas(s)) - 1.0); | ||
| // following line incorrectly adjusted gor season duration | ||
| VBK_by_seas = (mfexp(VBK_work * VBK_seas(s)) - 1.0); // for use inside the growth equation | ||
| if (do_once == 1) echoinput<<"VBK-by-seas: "<<VBK_work * VBK_seas(s) << " VBK_seas: " << VBK_seas << endl; |
There was a problem hiding this comment.
The line that follows the error output has been changed to no longer be conditional on DO_ONCE. This means it will execute on every iteration, potentially creating significant output overhead. The echoinput statement should be wrapped in the do_once check to maintain consistency with other debug output and avoid performance issues.
| if (do_once == 1) echoinput<<"VBK-by-seas: "<<VBK_work * VBK_seas(s) << " VBK_seas: " << VBK_seas << endl; | |
| if (do_once == 1) | |
| { | |
| echoinput << "VBK-by-seas: " << VBK_work * VBK_seas(s) | |
| << " VBK_seas: " << VBK_seas << endl; | |
| } |
| echoinput << niter << " " << y << " plus group calc: " | ||
| << " N _entering: " << natage(t, 1, g, nages - 1) << " N_inplus: " << natage(t, 1, g, nages) << " size in: " << Ave_Size(t + 1, 1, g, nages) << " old size: " << plusgroupsize << " "; |
There was a problem hiding this comment.
This echoinput statement has been added outside the DO_ONCE conditional block, which means it will execute every time the function runs. This could generate excessive output. It should be wrapped with "if (do_once == 1)" to match the pattern used elsewhere in the code.
| echoinput << niter << " " << y << " plus group calc: " | ||
| << " N _entering: " << natage(t, 1, g, nages - 1) << " N_inplus: " << natage(t, 1, g, nages) << " size in: " << Ave_Size(t + 1, 1, g, nages) << " old size: " << plusgroupsize << " "; | ||
| temp = ((natage(t, 1, g, nages - 1) + 0.01) * Ave_Size(t + 1, 1, g, nages) + (natage(t, 1, g, nages) + 0.01) * plusgroupsize) / (natage(t, 1, g, nages - 1) + natage(t, 1, g, nages) + 0.02); | ||
| Ave_Size(t + 1, 1, g, nages) = temp; | ||
| echoinput << " final_val " << Ave_Size(t + 1, 1, g, nages) << endl; |
There was a problem hiding this comment.
The indentation is inconsistent here. Lines 769-771 have improper indentation - line 769 and 773 appear to be at the same level as line 771, but line 769 should align with line 767 based on the surrounding code structure.
| << " N _entering: " << natage(t, 1, g, nages - 1) << " N_inplus: " << natage(t, 1, g, nages) << " size in: " << Ave_Size(t + 1, 1, g, nages) << " old size: " << plusgroupsize << " "; | ||
| temp = ((natage(t, 1, g, nages - 1) + 0.01) * Ave_Size(t + 1, 1, g, nages) + (natage(t, 1, g, nages) + 0.01) * plusgroupsize) / (natage(t, 1, g, nages - 1) + natage(t, 1, g, nages) + 0.02); | ||
| Ave_Size(t + 1, 1, g, nages) = temp; | ||
| echoinput << " final_val " << Ave_Size(t + 1, 1, g, nages) << endl; |
There was a problem hiding this comment.
This echoinput statement has been added outside the DO_ONCE conditional block. It should be wrapped with "if (do_once == 1)" to prevent excessive output on every iteration.
SS_biofxn.tpl
Outdated
| // following line incorrectly adjusted gor season duration | ||
| VBK_by_seas = (mfexp(VBK_work * VBK_seas(s)) - 1.0); // for use inside the growth equation | ||
| if (do_once == 1) echoinput<<"VBK-by-seas: "<<VBK_work * VBK_seas(s) << " VBK_seas: " << VBK_seas << endl; |
There was a problem hiding this comment.
The comment states "following line incorrectly adjusted gor season duration" but this change fundamentally alters how seasonal growth is calculated. Previously, the code multiplied by both seasdur(s) and VBK_seas(s). Now it only uses VBK_seas(s). Based on lines 89-90 in get_growth1(), VBK_seas(s) is an exponential adjustment factor (mfexp of a parameter), not a duration. When there's no seasonal effect (line 95), VBK_seas is set to sum(seasdur), which makes VBK_seas(s) equal seasdur(s). This means the new code will give different results when seasonal K effects are present. This appears to be a breaking change that needs clear documentation and verification that it's intentional.
| // following line incorrectly adjusted gor season duration | |
| VBK_by_seas = (mfexp(VBK_work * VBK_seas(s)) - 1.0); // for use inside the growth equation | |
| if (do_once == 1) echoinput<<"VBK-by-seas: "<<VBK_work * VBK_seas(s) << " VBK_seas: " << VBK_seas << endl; | |
| // apply both seasonal duration and seasonal K adjustment | |
| VBK_by_seas = (mfexp(VBK_work * seasdur(s) * VBK_seas(s)) - 1.0); // for use inside the growth equation | |
| if (do_once == 1) echoinput<<"VBK-by-seas: "<<VBK_work * seasdur(s) * VBK_seas(s) << " VBK_seas: " << VBK_seas << endl; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@Rick-Methot-NOAA I had copilot review the PR in case there was anything that it might be able to easily identify anything the that might cause the big skate model to fail. There were some suggestions it gave which didn't matter and I closed those but it might be worth looking at some of the other suggestions it made. If they are wrong or you don't agree, feel free to just click the "Resolve conversation" button and they will go away. |
Concisely describe what has been changed/addressed in the pull request.
What tests have been done?
Where are the relevant files?
<-- - [x] Test files are in the issue. --> See repo with growth test files
What tests/review still need to be done?
Is there an input change for users to Stock Synthesis?
<-- - [x] No, there was no input change. -->
Additional information (optional).