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

Move length-weight estimation function into this package? #110

Open
iantaylor-NOAA opened this issue Feb 3, 2023 · 5 comments
Open

Move length-weight estimation function into this package? #110

iantaylor-NOAA opened this issue Feb 3, 2023 · 5 comments

Comments

@iantaylor-NOAA
Copy link
Contributor

iantaylor-NOAA commented Feb 3, 2023

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

@chantelwetzel-noaa
Copy link
Contributor

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.

@kellijohnson-NOAA
Copy link
Contributor

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.

@iantaylor-NOAA
Copy link
Contributor Author

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

@iantaylor-NOAA
Copy link
Contributor Author

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.

@chantelwetzel-noaa
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants