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

add offset option for ENV Q #596

Merged
merged 11 commits into from
May 20, 2024
Merged

add offset option for ENV Q #596

merged 11 commits into from
May 20, 2024

Conversation

Rick-Methot-NOAA
Copy link
Collaborator

What tests have been done?

Where are the relevant files?

What tests/review still need to be done?

Is there an input change for users to Stock Synthesis?

Additional information (optional).

@Rick-Methot-NOAA Rick-Methot-NOAA marked this pull request as ready for review April 30, 2024 18:37
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. Thanks for adding this valuable feature. It does have an impact on the petrale model as expected.

The only confusion I have is in how the calculation of expected index value works.
Based on the code, I would have thought that Exp = Q_base * (Vuln_bio + Q_offset), but when I attempt that calculation from the INDEX_2 output in the attached spreadsheet using the example model attached to #596, I get a difference of 0.06549 for all years of the index.

Spreadsheet: env_index_calcs.xlsx

{
if (Q_setup(f, 1) != 5)
{
warnstream << "Suggest using Q option 5 to include offset parameter for an ENV index";
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change for an ENV index to for an index of deviations (type 35 or 36) to be both more general and more explicit on which types the recommendation is for.

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 code was done same as for Power function:

                if (Q_setup(f, 1) == 5) //  add offset
                {
                  vbio += Q_parm(Q_setup_parms(f, 1) + 1);
                }

                // SS_Label_Info_46.1.1 #note order of operations,  vbio raised to a power, then constant is added, then later multiplied by Q.  Needs work
                if (Q_setup(f, 1) == 3) //  link is power function
                {
                  vbio = pow(vbio, 1.0 + Q_parm(Q_setup_parms(f, 1) + 1));
                } //  raise vbio to a power

                Svy_selec_abund(f, j) = vbio;  //  e.g. the abundance that has been selected through selectivity or other assignment process

Which is then presented as available biomass in table Index_2, Which is confusing. Better to save selex_abund untransformed (e.g. selectivity only), then expected values will have q applied. I'll check to see if that works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ian-taylor I think I need to deprecate the output column eff_Q which was simply svy_est / svy_selec_abund. It sort of worked when svy_selec_abund was vbio because it included effect of the offset. Now with the offset it is a misleading output. I think best to deprecate it and put NA in the column.

Copy link
Contributor

Choose a reason for hiding this comment

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

The numbers now line up more intuitively, so thanks for making the change.

The only place that I've used eff_Q was looking at the change in Q as a function of vulnerable biomass (or what I thought was vbio) to see the effect of a power function on catchability. If there's another way to get that information, I'm OK with eff_Q going away. Just to clarify, would it only go away for links 5 and 6 or for all links (in which case I think the column could be removed).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

report now has NA if offset is used. Also, values of offset and power now added as new columns for INDEX_1 table, with NA as appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Expanded table looks good. Existing r4ss code showing vulnerable biomass vs. effective catchability continues to work for a model that used power function on Q with lognormal index error. Happy to have this squashed and merged assuming tests pass.

@e-perl-NOAA
Copy link
Collaborator

@Rick-Methot-NOAA Is there anything that needs to be updated in the documentation for this?

@Rick-Methot-NOAA
Copy link
Collaborator Author

Rick-Methot-NOAA commented May 3, 2024

@e-perl-NOAA The google doc labelled catchability is intended to replace the existing manual text.

@Rick-Methot-NOAA Rick-Methot-NOAA merged commit 8d6be9f into main May 20, 2024
11 checks passed
@Rick-Methot-NOAA Rick-Methot-NOAA deleted the Q-with-offset branch May 20, 2024 16:35
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.

[Feature]: Add 2-parm catchability (Q) option with offset and slope
3 participants