Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Refactor]: remove unused variables related to ALK? #415

Closed
iantaylor-NOAA opened this issue Mar 2, 2023 · 4 comments · Fixed by #417
Closed

[Refactor]: remove unused variables related to ALK? #415

iantaylor-NOAA opened this issue Mar 2, 2023 · 4 comments · Fixed by #417
Assignees
Labels
resolved issue resolved, look for "needs test" label
Milestone

Comments

@iantaylor-NOAA
Copy link
Contributor

Refactor request

While discussing workflows with @k-doering-NOAA yesterday, I looked at the artifact generated by the call-build-ss3-warnings workflow https://github.com/nmfs-stock-synthesis/stock-synthesis/actions/workflows/call-build-ss3-warnings.yml and discovered that the number of warnings has significantly reduced, presumably thanks to changes between ADMB version.

The remaining warnings are shown in the warnings_ss_ref.txt file available here: https://github.com/nmfs-stock-synthesis/workflows/blob/main/reference_files/warnings_ss_ref.txt (which is copied from the artifact from the most recent github action run on the main branch a few days ago).

The warnings all seem to be related to unused variable ‘ALK_finder’, as well as unused variable ‘ALK_finder’, unused parameter ‘ALK_range_lo’, and unused parameter ‘ALK_range_hi’. I presume that these are unused after deprecating the ALK tolerance code in Pull Request #342. If there is a good reason to keep there around, there's no problem ignoring the warnings, but if we can make the warnings go away, it will be that much easier to notice new warnings that appear in the future.

Expected behavior

Have even fewer warnings.

@Rick-Methot-NOAA
Copy link
Collaborator

Neal,
can you just comment out the creation of these variables? Or is it more complicated?

@nschindler-noaa
Copy link
Contributor

Yes. right away.

@nschindler-noaa
Copy link
Contributor

This is in main?

@nschindler-noaa
Copy link
Contributor

created branch remove-unused-ALK-variables commented them out.

@Rick-Methot-NOAA Rick-Methot-NOAA linked a pull request Mar 11, 2023 that will close this issue
2 tasks
@Rick-Methot-NOAA Rick-Methot-NOAA added resolved issue resolved, look for "needs test" label and removed new request initial entry of a new request labels Mar 11, 2023
@Rick-Methot-NOAA Rick-Methot-NOAA added this to the 3.30.22 milestone Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolved issue resolved, look for "needs test" label
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants