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

issue 646 - fix output error in Index_2, plus two minor output tweaks #647

Conversation

Rick-Methot-NOAA
Copy link
Collaborator

Concisely describe what has been changed/addressed in the pull request.

fixes output error in Index_2 that was introduced with Qtype 6.

What tests have been done?

Where are the relevant files?

[x] Test files are in the issue.

What tests/review still need to be done?

None

Is there an input change for users to Stock Synthesis?

[x] No, there was no input change.

Additional information (optional).

@Rick-Methot-NOAA Rick-Methot-NOAA linked an issue Nov 29, 2024 that may be closed by this pull request
@Rick-Methot-NOAA Rick-Methot-NOAA marked this pull request as ready for review November 29, 2024 21:59
@Rick-Methot-NOAA
Copy link
Collaborator Author

Ian,
I may have changed a table header that is affecting r4ss

iantaylor-NOAA added a commit to r4ss/r4ss that referenced this pull request Dec 2, 2024
Copy link
Contributor

@iantaylor-NOAA iantaylor-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. Thanks for fixing the issue so quickly.

The "Q" to "Qbase" change wasn't the issue for r4ss, rather the logic for parsing the INDEX_2 table wasn't adaptable to the additional comment line (unlike most other tables). I think I fixed the r4ss issue on the second try and just restarted the failed github action for the second time. If it passes the check (should be done in about 15 minutes) I think it's fine to merge.

@iantaylor-NOAA
Copy link
Contributor

The test-r4ss-with-ss3 check is now passing, so I think this is fine to merge.

@e-perl-NOAA do you know why build-ss3 was skipped?

@Rick-Methot-NOAA, do you think this bug needs a bug-fix release? It seems like users interested in Q_type 3 = power (+1 parm) which is causing the error fixed by this PR. could temporarily use type 6=offset & power (+2p) with the offset parameter fixed at 0 and get all the functionality of type 3. If we announced that solution on the forum we could potentially avoid a bug fix release and just wait until other features are ready for a bigger release.

@Rick-Methot-NOAA
Copy link
Collaborator Author

I thought the same Ian. Let's do a bug fix release for this one.

@e-perl-NOAA
Copy link
Collaborator

I have no idea why it was skipped, but I'm running it here

@Rick-Methot-NOAA
Copy link
Collaborator Author

Let's do both a big fix and a message in the Forum. The Forum message can serve to advertise the availability of the offset approach.

@iantaylor-NOAA
Copy link
Contributor

Sounds good.
A bug fix release has the additional benefit that we can test whether the linux and mac versions continue to have issues with missing libraries when run in those same platforms on github actions (necessitating an extra setup step to add the libraries as @e-perl-NOAA did at nmfs-ost/ss3-user-examples@7173d5e).

@e-perl-NOAA
Copy link
Collaborator

Sounds good!

@Rick-Methot-NOAA Rick-Methot-NOAA merged commit 3a25265 into main Dec 4, 2024
15 checks passed
@Rick-Methot-NOAA Rick-Methot-NOAA deleted the 646-bug-model-crash-during-output-stage-when-using-qpower branch December 4, 2024 19:00
@e-perl-NOAA e-perl-NOAA modified the milestones: 3.30.23.1, 3.30.23 Dec 17, 2024
@e-perl-NOAA e-perl-NOAA added change log use for issues that should appear in change log and removed change log use for issues that should appear in change log labels Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

[Bug]: model crash during output stage when using Qpower
3 participants