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

PIFSC opakapaka case study #23

Merged
merged 26 commits into from
May 13, 2024
Merged

PIFSC opakapaka case study #23

merged 26 commits into from
May 13, 2024

Conversation

MOshima-PIFSC
Copy link
Contributor

I am mostly finished with the Opaka case study. The FIMS model matches pretty well to the SS3 model. Some more comparisons between the models could be added in the future but I wanted to get what I have so far added to the case studies website.

@iantaylor-NOAA
Copy link
Contributor

@MOshima-PIFSC, nice job putting this case study together!

I resolved a trivial merge conflict in README which allowed the github action to render the quarto site. It seems to be successfully rendering the PIFS-opakapaka.qmd but failed in the petrale example because the modified get_ss3_data() function needs to the arguments all specified.

Could you make the following changes, which I think would get it to render successfully?

MOshima-PIFSC and others added 8 commits May 7, 2024 08:20
Update code to match redundant get_ss3_data.r file
source get_ss3_data.R
add `ages = ages` argument to `get_ss3_data()`
add `ages = ages` to `get_ss3_data()`
Because of redundancy with get_ss3_data.R
add ages param to roxygen
@MOshima-PIFSC
Copy link
Contributor Author

Thanks @iantaylor-NOAA for your review. I made the changes you requested, however, the petrale case study is now giving this error

Error in nlminb(obj$par, obj$fn, obj$gr, control = list(eval.max = 10000,  : 
  NA/NaN gradient evaluation
In addition: Warning message:
In nlminb(obj$par, obj$fn, obj$gr, control = list(eval.max = 10000,  :       
  NA/NaN function evaluation

I'm not positive what is causing this but it could be because of the changes I made to index negative fleets in get_ss3_data(). I tried modifying the function (only locally, didn't commit the changes to the repo) to allow it to accept negative fleet numbers (then it could filter out the rows of FltSvy == 4) but that doesn't seem to help. Let me know if you have any ideas of what to try.

@MOshima-PIFSC MOshima-PIFSC requested a review from Bai-Li-NOAA May 9, 2024 18:06
@Bai-Li-NOAA
Copy link
Contributor

Thanks @MOshima-PIFSC for submitting a new case study!

@iantaylor-NOAA, I think the error in the GHA log is related to a mismatch in input data dimensions (e.g., number of years) in the petrale case study. I've made some changes in the fix-petrale-case-study branch for demonstration purposes. These changes will enable us to run FIMS successfully, but you will still see errors in the "Get FIMS output and plot results" section. I'm not sure about the correct number of years, so please ignore any hardwired years from the demonstration. Feel free to ping me when you think the pull request is ready for review.

@iantaylor-NOAA
Copy link
Contributor

Thanks @Bai-Li-NOAA, for catch the mismatched years are the issue and also the transformation of the SS3 log(R0) parameter.

@MOshima-PIFSC generalized the get_ss3_data() to have fleets as an argument (instead of my sloppy use of a variable created outside the function). However, filtering the catch by fleet, combined with the use of the catch time series as the source for the range of years in the model caused things to break. I removed that filtering because it looks like the SS3 opakapaka model only has catch for fleet 1 so filtering to just fleets 1 and 2 wouldn't have had an impact. This has gotten the petrale model to run successfully through the nlbinb step (though there's still an error in the output that I'm debugging now).

I will copy changes from the fix-petrale-case-study branch along with the debugging of the plotting.

If we want to generalized get_ss3_data() in the future, it might be good to apply the fleet filtering after converting into FIMS format or provide separate arguments for fleets to use for which data types.

@iantaylor-NOAA
Copy link
Contributor

It took me a while to clean up the mess with the petrale case study in this Pull Request. There were weird things going on with the filtering and converting negative fleet numbers in the SS3 data (both in the logic and because I forgot about some of the data that were being excluded from the likelihood) so I just moved all that processing into the petrale case study and simplified the filtering in the get_ss3_data(). Now the petrale results match what they looked like previously but with simpler code. And Opakapaka continues to work as intended, I think.

Thanks @MOshima-PIFSC and @Bai-Li-NOAA for your work on this.
I don't know if either of you want to look at the remaining changes. If not, feel free to merge.

@MOshima-PIFSC
Copy link
Contributor Author

Thanks @iantaylor-NOAA for doing so much work for this (sorry I broke your code 😬 )! I double checked the opaka code and it still works fine, so no worries on my end. I will let @Bai-Li-NOAA merge this PR.

Copy link
Contributor

@Bai-Li-NOAA Bai-Li-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. I have merged the changes into the main branch.

@Bai-Li-NOAA Bai-Li-NOAA merged commit c4a9835 into main May 13, 2024
1 check passed
@Bai-Li-NOAA Bai-Li-NOAA deleted the PIFSC-opakapaka branch May 13, 2024 13:09
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.

3 participants