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

Add new functions to calculate expanded composition data and input sample sizes #145

Open
wants to merge 63 commits into
base: main
Choose a base branch
from

Conversation

chantelwetzel-noaa
Copy link
Contributor

New functions:

  1. get_expanded_comps() expands length or age composition data to the tow and strata level. This new function replaces the SurveyLFs.fn() and SurveyAFs.fn(). The new function calculates the expanded composition data in the same manner as the existing functions but more streamlined. If two_sex_model = TRUE and output = "full_expansion_ss3_format" in the get_expanded_comps(), then a list of sexed and unsexed composition data is returned. This new function also includes an input_sample_size_method input and will automatically calculate the input sample size.
  2. get_input_n() calculates the input sample size for composition data and is designed to replace the GetN.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 within get_expanded_comps() that includes a function input input_sample_size_method which will add input sample size to the formatted Stock Synthesis composition data.
  3. The get_raw_comps() is modified to have similar function inputs as get_expanded_comps() to improve consistency and to support alternative approaches to calculate input sample size within the function.
  4. get_species_info() is designed to replace GetSpp.fn(). The get_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.

@kellijohnson-NOAA
Copy link
Contributor

@chantelwetzel-noaa I have set aside time both tomorrow and Friday to look at this.

@chantelwetzel-noaa
Copy link
Contributor Author

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.

@kellijohnson-NOAA
Copy link
Contributor

{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 😃!

R/get_input_n.R Outdated Show resolved Hide resolved
@chantelwetzel-noaa
Copy link
Contributor Author

@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
Copy link
Contributor

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.

Copy link
Contributor Author

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
Comment on lines 76 to 77
cli::cli_abort(
"The {missing_columns} is not a column name in the data.")
Copy link
Contributor

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."
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@chantelwetzel-noaa
Copy link
Contributor Author

@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.

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