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

adjust warnings for 2DAR #617

Conversation

Rick-Methot-NOAA
Copy link
Collaborator

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

Find that the rho feature of 2DAR was incompletely implemented and a full fix is not feasible at this time. Provide new warnings and guidance to users.

What tests have been done?

Numerous runs with various permutations of estimating or fixing age-specific sigmasel values using hake 2DAR example.

Where are the relevant files?

What tests/review still need to be done?

none

Is there an input change for users to Stock Synthesis?

Additional information (optional).

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.

Looks good to me. Thanks for all the explorations.

SS_selex.tpl Show resolved Hide resolved
@Rick-Methot-NOAA Rick-Methot-NOAA merged commit 605a4c2 into main Sep 13, 2024
9 of 10 checks passed
@Rick-Methot-NOAA Rick-Methot-NOAA deleted the 615-bug-rho-parameters-not-impacting-semi-parametric-2d-ar-selectivity-when-multiple-sigma-values-are-used branch September 13, 2024 19:13
@Rick-Methot-NOAA
Copy link
Collaborator Author

@e-perl-NOAA Please reflect these revised warnings into the manual.

Copy link
Contributor

@kellijohnson-NOAA kellijohnson-NOAA left a comment

Choose a reason for hiding this comment

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

I am uncertain of the level of guidance that you should be giving the user. It seems like you are just warning them and then changing it behind the scenes, where I prefer that the model just stops if they try to estimate it.

}
if (use_rho == 1)
{
echoinput << "read two parameter lines for rho_yr and then rho_age (or length)" << endl;
warnstream << "2DAR rho is incompletely implemented; it should only be used experimentally and never estimated";
Copy link
Contributor

Choose a reason for hiding this comment

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

I am hesitant to allow users to even do this if it is incompletely implemented. I would prefer a full stop if use_rho = 1. Especially because in this same if statement you set dtempvec(7) = -1. Which, does that just change the values put in ss_new or does it actually turn off estimation without the user knowing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The estimation has never been enabled. The value of rho is only used in the preliminary calcs section to create a cor matrix.

@Rick-Methot-NOAA Rick-Methot-NOAA restored the 615-bug-rho-parameters-not-impacting-semi-parametric-2d-ar-selectivity-when-multiple-sigma-values-are-used branch September 13, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: rho parameters not impacting semi-parametric (2D-AR) selectivity when multiple sigma values are used
3 participants