-
Notifications
You must be signed in to change notification settings - Fork 8
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 new functions to calculate expanded composition data and input sample sizes #145
base: main
Are you sure you want to change the base?
Conversation
This function would eventually replace the SurveyLFs.fn and the SurveyAFs.fn functions.
1. Will eventually replace the GetSpp.fn 2. Add a column that added a species group based on Stewart and Hamel input sample size calculation.
1. Will eventually replace GetN.fn. 2. Reports out the number of unique tows, samples, and the Stewart and Hamel sample size for sexed, unsexed, and all samples.
@chantelwetzel-noaa I have set aside time both tomorrow and Friday to look at this. |
Thank you. I was looking at the issue list for the package and saw the issue that suggests moving warning messages to the cli package. I would be happy to also incorporate that change in this pull request if you agree with that move. |
{cli} is amazing, it is what sdmTMB uses. I have pirated some of their code for learning how to do messages and warnings in ss3sim this way 😃! |
@kellijohnson-NOAA I have worked through all your comments and made changes based on most of them. There are some questions in my responses. The two key ones are 1) when should I apply a new tag? and 2) how to delete and transfer a commit history for a file? Let me know if you find new issues or issues that you would like looked at again. |
R/get_input_n.R
Outdated
#' | ||
#' This function calculates the number of unique tows, samples, and input sample | ||
#' size based on alternative approaches. The input sample size then is used | ||
#' within the `get_expanded_comps()` and `get_raw_comps()` when creating formatted |
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.
These two functions that are referenced should be formatted as [get_expanded_comps()] and [get_raw_comps()] so that the links are created rather than using ...
because that just makes them look like code. Additionally, you could put this information in the @Seealso tag but it is fine here too.
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.
Done.
R/get_input_n.R
Outdated
cli::cli_abort( | ||
"The {missing_columns} is not a column name in the data.") |
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.
You can add pluralism to the call to cli::cli_abort()
via glue. See below.
cli::cli_abort(
"The {.val {missing_columns}} {cli::qty(missing_columns)} column{?s} {?is/are} not in the data."
)
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.
Done.
@kellijohnson-NOAA I think I have gone back and addressed each of your new comments. I opened an issue to circle back to using helper functions in the future once I have time to fully understand how the code works and have time to address some minor issues in the functionality. I think the only two outstanding items are how to delete a file while transferring the commit history to the replacement function and when to apply a new tag to the package. Thanks. |
rename file update file
New functions:
get_expanded_comps()
expands length or age composition data to the tow and strata level. This new function replaces theSurveyLFs.fn()
andSurveyAFs.fn()
. The new function calculates the expanded composition data in the same manner as the existing functions but more streamlined. Iftwo_sex_model = TRUE
andoutput = "full_expansion_ss3_format"
in theget_expanded_comps()
, then a list of sexed and unsexed composition data is returned. This new function also includes aninput_sample_size_method
input and will automatically calculate the input sample size.get_input_n()
calculates the input sample size for composition data and is designed to replace theGetN.fn
. This function outputs a table that includes the number of unique tows, total samples, and the input sample size based on the Stewart and Hamel approach by composition type (sexed or unsexed). This function is now called withinget_expanded_comps()
that includes a function inputinput_sample_size_method
which will add input sample size to the formatted Stock Synthesis composition data.get_raw_comps()
is modified to have similar function inputs asget_expanded_comps()
to improve consistency and to support alternative approaches to calculate input sample size within the function.get_species_info()
is designed to replaceGetSpp.fn()
. Theget_species_info()
adds a column to the dataframe output to specify the Stewart and Hamel species group for determining input samples sizes by species type.The soft deprecation warning references a new tag number that still needs to be added. I can never remember if the tag should be added in the branch being merged in or should be added to the main branch post-merge. Please let me know your preference and I will deal with that.