Move length-weight estimation function into this package?
iantaylor-NOAA opened this issue · 8 comments
{nwfscSurvey} includes fit_vbgrowth(), while {PacFIN.Utilities} has getWLpars().
I think a common assumption in PFMC assessments is that the survey data is the best source for the length-weight relationship used in SS3 models because it's more representative of the population as a whole, and the function would be easier to find if it were located here.
{PacFIN.Utilities} already depends on {nwfscSurvey}, so I think the change would only require an extra nwfscSurvey:: to make use of the function.
I think that is reasonable. I don't know if I have ever used the function in {PacFIN.Utilities} and looking at that code it is definitely a @kellijohnson-NOAA creation with all the tidyr, dplyr, and purring occurring. I will put this on the to-do list for the package.
Sorry for being dense but I am uncertain what the desired outcome is here? Is the desired outcome to move getWLpars() to {nwfscSurvey} and keep nwfscSurvey::fit_vbgrowth() as well? I am all for functions that perform tasks desired outside of {PacFIN.Utilities} to be moved elsewhere or be made more available. I am also fine, if the former is to be moved, with changing the name to fit_*() because I am trying to rename things such that get_*() means getting information from somewhere else and revamp camel-case names to snake case.
@kellijohnson-NOAA, your question gets at some bigger picture design questions about how these packages relate.
You and @chantelwetzel-noaa know better, but I gather from previous conversations that a plan for PacFIN.Utilities is to focus on the extraction, cleaning, and expansion of PacFIN data whereas nwfscSurvey will extract survey data but also include the more generalized functions related to binning data, getting into SS3 input format, etc. that could be applied to data from any source. Both fit_vbgrowth() and getWLpars() seems to fit better within that umbrella of nwfscSurvey().
An alternative would be to create a third package which contains the fit_*() functions, but that seems overkill at this stage.
As for alternative function names, I would vote for fit_length_weight() to keep with the fit_*() pattern and be similar to estimate_length_weight() that @chantelwetzel-noaa has used in the past for a different implementation).
Additional note that if/when this function is moved, we can also change from dplyr::summarise() to dpyr::reframe() based on the warning below:
> WLpars <- getWLpars(Pdata)
Warning message:
Returning more (or less) than 1 row per `summarise()` group was deprecated in dplyr 1.1.0.
i Please use `reframe()` instead.
i When switching from `summarise()` to `reframe()`, remember that `reframe()` always returns an ungrouped data frame and adjust accordingly.
i The deprecated feature was likely used in the PacFIN.Utilities package.
Please report the issue to the authors.
@iantaylor-NOAA Thank you for the warning about changing dplyr functions. I plan on moving this function over to the {nwfscSurvey} package tomorrow. Since we are assessment season now, I don't plan on removing the existing from {PacFIN.Utilities} but rather revising and adding it the survey package with a new function name. Once we are convinced that the new function adequately replaces the old one, we can deprecate and/or call the new survey package function within the old one.