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

skip maturity- & fecundity-at-length calculations when empirical values at age are input directly #348

Closed
iantaylor-NOAA opened this issue Jul 12, 2022 · 6 comments · Fixed by #349
Labels
biology change log use for issues that should appear in change log misc. internal calc
Milestone

Comments

@iantaylor-NOAA
Copy link
Contributor

@latreesedenson-NOAA and @Matthew-Smith-NOAA reported an r4ss plotting issue r4ss/r4ss#709 with their red snapper model which I tracked down as related to "Inf" values in the BIOLOGY table.

I replicated the issue in the attached modification of "Simple".

When maturity option 4=read age-fecundity is combined with placeholder 999 values for the maturity and fecundity parameters, fecundity-at-length is calculated as 999 * weight ^ 999 though these values have are not used in the model.

I can make a fix to r4ss to skip the plots of length-based maturity and fecundity when maturity option 4 is used, but could we also report "NA" values for the "Mat_len", "Spawn", and "Fecundity" columns in BIOLOGY for models that use age-based maturity options or empirical weight-at-age? NA could be reported even if the fecundity is still being calculated, but I supposed skipping the calculation could also be nice (though presumably not very time-intensive if it only happens once).

An additional option would be to also skip the input of the parameters lines, saving users from adding placeholder values, but I assume that would take a lot more work.

Simple_maturity4.zip

@iantaylor-NOAA iantaylor-NOAA added misc. internal calc quick few hours work r4ss r4ss may need changes based on this issue. new request initial entry of a new request labels Jul 12, 2022
@Rick-Methot-NOAA
Copy link
Collaborator

I've been wanting to bypass a bunch of calculations like this when doing empirical wt-at-age. Add this one to the list. Let's prioritize it.

@Rick-Methot-NOAA
Copy link
Collaborator

My preference is to do the code modification where these quantities are first calculated in ss_biofxn.tpl's get_mat_fec() function.

I think setting the vector to all 0.5 values would be better than all 0.0 or 1.0, but open to suggestions there. Cannot set to NA because they are numeric.

The code is already bypassing the mat fec vector calculations when WTage_rd == 1. So no action needed there, but perhaps the length-based quantities could also be set to 0.5 when this option is used.

@Rick-Methot-NOAA Rick-Methot-NOAA added the in progress This is being worked on in a branch label Jul 13, 2022
@Rick-Methot-NOAA
Copy link
Collaborator

Rick-Methot-NOAA commented Jul 13, 2022

Biology report now looks like this if Maturity_Option is 4 or 5:
BIOLOGY report:42
2 41 60 1 N_Used_morphs;_lengths;_ages;_season;_by_season_in_endyr
GP Bin Low Mean_Size Wt_len_F Mat_len Spawn Wt_len_M Fecundity // [Mat@Len, Spawn (Mat_Fec@Len), and Fecundity@Len reported as 0.5 because maturity option directly reads age_maturity]
1 1 5 6 0.00352655 0.5 0.5 0.00332761 0.5
1 2 7 8 0.00838181 0.5 0.5 0.00798531 0.5

Note that I am open to changing the column headers to:
GP Bin Len_lo Len_mean Wt_F Mat Mat*Fec Wt_M Fec
because all are @len
but don't want to create extra work for r4ss

@iantaylor-NOAA
Copy link
Contributor Author

I think the revised headers are a good idea. What we can do in r4ss is just rename the old names, if present, to the new names and base all plotting code on the new names. That's how we've been dealing with changes like this in recent years and it works well, especially when there's a 1-to-1 match of old to new.

@iantaylor-NOAA
Copy link
Contributor Author

Thanks for working on this Rick. I just created an r4ss branch https://github.com/r4ss/r4ss/tree/mat_fec_cleanup to adapt to the new column names, which look good to me.

@k-doering-NOAA, can you advise on two points:

  1. shouldn't the pull request clean up and add comments for mat_fec calcs #349 have automatically created an r4ss issue?
  2. what's the best way to test the new SS3 branch with the new r4ss branch, which might also require a new branch in the workflow repo?

On point 2, the r4ss branch should actually work fine with the status-quo SS3. That is, as long as I consistently changed the column names throughout, everything should still work using the new names even if they aren't being output by SS3 yet. If it passes the tests we could merge it into "main" and then we can just restart github actions for the new SS3 branch. But for future reference we may need to think of the best way to make parallel changes across 2 or 3 repos which will be reliable and not require too much extra work.

@k-doering-NOAA
Copy link
Contributor

@iantaylor-NOAA Thanks for asking these questions!

  1. shouldn't the pull request clean up and add comments for mat_fec calcs #349 have automatically created an r4ss issue?
    The issues are created when the pull request is merged in rather than when it is created, which is why no issue was created.
  2. what's the best way to test the new SS3 branch with the new r4ss branch, which might also require a new branch in the workflow repo?

I think everything you brought up makes sense and how I would do it! I'm not sure there is a good way besides thinking carefully about how to coordinate changes. As a general philosophy, I would test in branches and merge in when you have high confidence that it will work.

@Rick-Methot-NOAA Rick-Methot-NOAA added this to the 3.30.20 milestone Sep 16, 2022
@Rick-Methot-NOAA Rick-Methot-NOAA added change log use for issues that should appear in change log resolved issue resolved, look for "needs test" label biology and removed quick few hours work r4ss r4ss may need changes based on this issue. new request initial entry of a new request in progress This is being worked on in a branch labels Sep 16, 2022
@Rick-Methot-NOAA Rick-Methot-NOAA changed the title skip maturity- & fecundity-at-length calculations when values at age are input directly? skip maturity- & fecundity-at-length calculations when empirical values at age are input directly Sep 22, 2022
@Rick-Methot-NOAA Rick-Methot-NOAA removed the resolved issue resolved, look for "needs test" label label Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
biology change log use for issues that should appear in change log misc. internal calc
Projects
Status: No status
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants