Feature(#113): add calc_condition function

[stephanie-owen]

Justification

Migration of condition calculation code from NEesp2 to survdat, as condition already incorporates multiple survdat functions and data. Allows for more streamlined applications in the SOE and ESPs with other similar survdat functions.

Fixes #113

Types of changes

• Fix (non-breaking change which fixes a bug)
• Feature (non-breaking change which adds or changes functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Other change (if none of the other choices apply)

Further comments

The survdat function (in comparison to previous NEesp2 function) has arguments for 'lengthweight' and 'aggregate_by', which allows the user to use either the LW parameters from Wigley et al. (2003) or from survdat::get_length_weight and the spatial scale either by strata or EPU.

In addition to calc_condition.R, I have added datasets for the strata/EPU key (data/EPUstrata.rda) and Wigley length weights (data/Wigley_LW.rda) to the package. Data documentation for survdat::Wigley_LW is included as an R script in data-raw. If there is a better place to house this documentation please let me know.

Reviewer instructions

• Run the following in this branch: feature/i113-add-condition-function. May need to run devtools::load_all() prior to the condition function.

• Connect to channel using channel <- dbutils::connect_to_database(server = "", uid = "")

• Pull survey data using data <- survdat::get_survdat_data(channel, getBio = TRUE, getLengths = TRUE)

• Test the calc_condition function using the following two calls. These test for both strata and EPU and Wigley and survdat options:

calc_condition(channel = channel, surveyData=data$survdat, lengthweight = "Wigley", aggregate_by = "EPU", by_sex = FALSE, length_break = NULL)

and

calc_condition(channel = channel, surveyData=data$survdat, lengthweight = "survdat", aggregate_by = "strata", by_sex = FALSE, length_break = NULL)

• Check documentation inR/calc_condition.R and data-raw/Wigley_LW_documentation.R for accuracy.

Formatting

This repo contains an air.toml file that automatically formats code to a set of standards.
It is preferred that contributors and reviewers install the Air formatting tool.
Code submitted in this pull request will be automatically checked for correct formatting.

[stephanie-owen]

@andybeet I ran roxygen2::roxygenise() to add the Wigley dataset survdat::Wigley_LW as an export of survdat, which created the merge conflicts in man. Please let me know if I need to undo/redo this to avoid the conflicts.

[andybeet]

As per our conversation
Once you add the data docs to the /R folder and add create_<dataset>.r to data-raw please re-request a review.

I'll take a look at those merge conflicts. I think that is either from an earlier issue we didnt address or maybe be are using different versions of roxygen. i have 8.1.0

[atyrell3]

Lines 34-44 need to be removed as the output parameter is no longer used so it's causing the function to break if by_sex or length_breaks aren't NULL.

I also suggest renaming column n to nYears.

Other than that, looks good!

[stephanie-owen]

2. suggestion(non-blocking): the two options for lengthweight - result in a different number of species being returned. If that is truly the case we might want to explain that in the documentation, maybe in its own section.

@andybeet Blueback herring and northern searobin are present in the survdat length weights but absent from Wigley. Should this go into the documentation in calc_condition.R under the 'lengthweight' parameter or under the data file in R/data-Wigley_LW.R?

[andybeet]

@stephanie-owen i think the species information should be with the data documentation rather than the function documentation. However, we could include a @section in the function documentation that just warns the use that the bundled data sets include different species. what do you think? Does that make sense?

[stephanie-owen]

@andybeet @atyrell

For re-review

Addressing the following suggested issues. All blocking issues and non-blocking issues in bold have been addressed in recent commits. Non-blocking issues in regular text could be added in subsequent versioning of the package.

Reviewer instructions at the bottom of issues list.

Documentation

Addressed all of the below documentation issues.

1. issue(blocking): #'#' @param channel is preventing proper build of documentation. Remove additional #'
2. issue(blocking): replace line with #' @param surveyData Data table. NEFSC survey data generated by [survdat::get_survdat_data()]. To create hyperlink to referenced function. This also involves adding Roxygen: list(markdown = TRUE) to DESCRIPTION file
3. issue(blocking): The Description of calc_condition() doesn't need a function call. The functions use is outline nicely in the examples.
4. issue(blocking): the 3 new items (calc_condition, and the 2 data sets) are not included in the pkgdown/pkgdown.yml. Run pkgdown::build_site() for details. I'd put the function in the Estimation section, and the datasets in the Sample Data section, but remaine the section to Data
5. issue(non-blocking): I'd suggest moving the reference to laurels work to section called "references"? you can use the tag @section for this
6. issue(non-blocking): Would be better to explain what the lengthweight argument is, specifically when selecting either of the options. This is also a character string not a data-table.
7. issue(non-blocking): Maybe add some additional examples to show how to use length_break

Function

1. issue(non-blocking): some of these arguments should have default values

Defaulted lengthweight to Wigley and aggregate_by to EPU so the function defaults to SOE and ESP usage.

2. issue(non-blocking): the original issue mentioned arguments for filtering by species, season, time frame. A flexible function for calculating condition. We should implement that so this isn't just a function with a single use (SOE_workflows), otherwise this function would reside there. Don't worry about this now.

3. issue(non-blocking): no longer a need for .data$ since moving to the native pipe. These can safely be removed

4. issue(non-blocking): might need some argument checks. eg. if aggregate_by = "strat" the function still runs. Not sure the user will know they made a typo, but the output is different from strata

5. issue(non-blocking): should be explicit in the left_join. eg add by = "STRATUM" to tell the function which fields to join by. Better to not leave dplyr to guess for you, even though it is usually correct

General

1. issue(non-blocking): We should also think about how we would handle the user wanting to pass their own length-weight info to the function, assuming they don't want to use either of the two options.

If we want to do this in the future, should add some requirements for columns and formatting for users using their own length-weights to ensure their data still works within the function.

2. suggestion(non-blocking): the two options for lengthweight - result in a different number of species being returned. If that is truly the case we might want to explain that in the documentation, maybe in its own section.

Added a @section in the function documentation and also a note in data-Wigley_LW that the Wigley LWs do not include blueback herring or searobin.

Reviewer instructions

• Connect to channel using channel <- dbutils::connect_to_database(server = "", uid = "")

• Pull survey data using data <- survdat::get_survdat_data(channel, getBio = TRUE, getLengths = TRUE)

• Test the calc_condition function using the following two calls. These test for both strata and EPU, Wigley and survdat options, and length break:

calc_condition(channel = channel, surveyData=data$survdat, lengthweight = "Wigley", aggregate_by = "EPU", by_sex = FALSE, length_break = NULL)

and

calc_condition(channel = channel, surveyData=data$survdat, lengthweight = "survdat", aggregate_by = "strata", by_sex = FALSE, length_break = c(0, 20)

• Check documentation inR/calc_condition.R for accuracy

[atyrell3]

The calc_condition helpfile wasn't updated, so I reknit it with roxygen. I also hyperlinked the SOE_workflows reference in EPUstrata.

Function works as expected, good to go!

[stephanie-owen]

For re-review

Merged PR 132 into this branch. Branch was created to modify documentation to correspond to similar functions and datasets in survdat. Nothing has changed regarding the way the function runs.

Reviewer Instructions

Pull latest changes
Build the package ctrl + shift + b
Build the website pkgdown::build_site()
Inspect the 3 associated pages: calc_condition, EPUstrata, Wigley_LW