-
Notifications
You must be signed in to change notification settings - Fork 16
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
Conversation
There was a problem hiding this 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
SS_readcontrol_330.tpl
Outdated
{ | ||
if (Q_setup(f, 1) != 5) | ||
{ | ||
warnstream << "Suggest using Q option 5 to include offset parameter for an ENV index"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@Rick-Methot-NOAA Is there anything that needs to be updated in the documentation for this? |
@e-perl-NOAA The google doc labelled catchability is intended to replace the existing manual text. |
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).