Dev two population vignette#1446
Conversation
This vignette introduces multi-population modeling in FIMS, detailing the setup, recruitment, growth, maturity, and catch-at-age models for two populations.
Updated the vignette to enhance clarity and structure, including detailed explanations of the workflow and model configurations for multi-population modeling in FIMS.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1446 +/- ##
==========================================
+ Coverage 82.97% 89.46% +6.49%
==========================================
Files 54 83 +29
Lines 2214 8478 +6264
Branches 579 582 +3
==========================================
+ Hits 1837 7585 +5748
- Misses 311 861 +550
+ Partials 66 32 -34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| caa$AddPopulation(population1$get_id()) | ||
| caa$AddPopulation(population2$get_id()) |
There was a problem hiding this comment.
You set up 2 populations but the vignette should go deeper than that to actually make sure that they work, e.g., that you can make at least one parameter different and that the results show how to look at the results of the parameters. I need to be able to see that this could be used for sex and right now I do not see that.
|
@kellijohnson-NOAA I wonder if there should be a section up at the top that defines/lists what vignettes should have been completed/reviewed before attempting to work through this one (this might be a good practice for most/all the vignettes?). I could just see people hopping straight into vignettes for the type of model that they need to build and skipping a lot of the fundamental info that is useful (such as what the heck using methods::new() is all about). |
| ## Inspect results | ||
|
|
||
| ```{r inspect-results} | ||
| # Gradient at the final parameter values. Values near zero indicate that the |
There was a problem hiding this comment.
Maybe break this up into three chunks, one for the obj$gr(), one for the opt, and one for the head(sdr_report) and add some text before each small chunk about what you want to draw the user's attention to in each of the results that you are providing.
|
|
||
| When adapting this example, check whether a module should be shared across | ||
| populations or instantiated separately. Sharing a module means the populations use | ||
| the same registered object and therefore the same parameterization. |
There was a problem hiding this comment.
You could insert something like
"(see the Defined shared recruitment section)"
at the end of this text so people can quickly refer to what it means to do that.
| recruitment$log_rzero[1]$value <- log(om_input[["R0"]]) | ||
| recruitment$log_rzero[1]$estimation_type$set("fixed_effects") | ||
|
|
||
| # Beverton-Holt steepness on the FIMS logit scale. |
There was a problem hiding this comment.
You started off with a lot of inline text throughout the code chunks that were super helpful, but it stops at some point throughout the vignette. If you could add more inline comments and/or text before broken up code chunks throughout the rest of the vignette, I think that would be helpful.
Updated the vignette to demonstrate a two-population FIMS model with distinct initial numbers at age for each population, interpreted as sexes. Adjusted workflow steps and added clarifications on population-specific parameters.
3 participants
What is the feature?
Instructions for code reviewer
👋Hello reviewer👋, thank you for taking the time to review this PR!
nit:(for nitpicking) as the comment type. For example,nit:I prefer using adata.frame()instead of amatrixbecause ...This PR is now ready to be merged.Checklist