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

Remove {knitr} from main code #96

Open
kellijohnson-NOAA opened this issue Feb 24, 2023 · 3 comments
Open

Remove {knitr} from main code #96

kellijohnson-NOAA opened this issue Feb 24, 2023 · 3 comments
Assignees
Labels
priority: low The lowest level priority, i.e., not urgent. status: in progress Currently working on this issue topic: code Related to R code within this package type: bug

Comments

@kellijohnson-NOAA
Copy link
Contributor

{knitr} is a suggests not an import and yet this table function in checkLenAge() uses {knitr}. I will try to find some other way to summarize the information.

x <- knitr::kable(estsall, format = "latex",

@kellijohnson-NOAA kellijohnson-NOAA added type: bug topic: code Related to R code within this package labels Feb 24, 2023
@kellijohnson-NOAA kellijohnson-NOAA self-assigned this Feb 24, 2023
@iantaylor-NOAA
Copy link
Contributor

I didn't realize that PacFIN.Utilities::checkLenAge() existed because the example {PacFIN.Utilities} script that @chantelwetzel-noaa provided uses nwfscSurvey::est_growth() to estimate von Bertalanffy growth curve and flag outliers (adapted for Petrale here: https://github.com/pfmc-assessments/petrale/blob/main/R/process_pacfin_bds.R#L149-L167).

Therefore, one potential solution would be to just remove PacFIN.Utilities::checkLenAge() (or initially deprecate but not require {knitr}) and rely on the {nwfscSurvey} version instead, similar to the plan proposed in pfmc-assessments/nwfscSurvey#110 for weight-length estimation.

@chantelwetzel-noaa
Copy link
Contributor

Given that we are mid-assessment season, I worried about getting rid of the PacFIN.Utilities::checkLenAge() could interrupt users workflow so opted to have duplication for the time being. However, if we wanted to clean things up we could call the nwfscSurvey::est_growth() from within the PacFIN.Utilities::checkLenAge().

@kellijohnson-NOAA
Copy link
Contributor Author

I will make sure to keep it ... and it already calls the same fit function that nwfscSurvey::est_growth() calls so basically the only differences are formatting of the output.

@kellijohnson-NOAA kellijohnson-NOAA added status: in progress Currently working on this issue priority: low The lowest level priority, i.e., not urgent. labels May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low The lowest level priority, i.e., not urgent. status: in progress Currently working on this issue topic: code Related to R code within this package type: bug
Projects
None yet
Development

No branches or pull requests

3 participants