-
Notifications
You must be signed in to change notification settings - Fork 2
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 do sex ratio and write comps more efficiently #135
base: main
Are you sure you want to change the base?
Conversation
Removed the loops in writeComps in favor of using dplyr and tried to align the column names to match what nwfscSurvey is outputting to make accounting easier. Added an example worflow for sablefish. Still need to work on sample size and ensure that the correct column is being used for the composition data. Thank you everyone for your patience.
Thanks @kellijohnson-NOAA for all your work on this package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kellijohnson-NOAA, I have only been able to skim through the code changes, but wanted to give you feedback on running through the vignette.
First the addition of the vignette is a really valuable step forward for this package. I had a few minor issues, some of which may be related to relying on July 2023 PacFIN data found in \nwcfile\FRAM\Assessments\CurrentAssessments\sablefish_2025\data-raw\ rather than the Nov 2024 data referenced in the vignette.
Here are the two issues that didn't have an obvious place in the code to comment on:
- Message when running
devtools::load_all()
:
Objects listed as exports, but not present in namespace:
* getcomps_long
- Not related to this PR except that it appears in the vignette, the different order of age methods in the following message had me initially thinking that they vectors didn't match.
i Age methods 'B', 'M', 'S' and 'NA' were present
i Age methods 'NA', 'B', 'S' and 'M' were desired
i 0 ages used undesired age methods
c( | ||
"i" = "sablefish uses HKL, POT, and TWL", | ||
"i" = "{change_to_hkl} are recoded to HKL.", | ||
"i" = "almost everything outside of the above is recoded to TWL." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change to something like "everything not assigned to HKL, other than POT, is recoded to TWL." to clarify (a) that POT is not assigned to TWL, and to remove the confusing "almost everything".
Pdata = bds_cleaned, | ||
maxExp = expansion, | ||
fa = weight_length_estimates |> | ||
dplyr::filter(group == "female") |> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below change from group
to sex
to match current output from nwfscSurvey::estimate_weight_length()
:
https://github.com/pfmc-assessments/nwfscSurvey/blame/84edde0b166e59489c314cf9cde91a38ffc40345/R/est_weight_length.R#L77
maxExp = expansion | ||
) | ||
|
||
data_exp2[["Final_Sample_Size"]] <- capValues( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running this, I got Maximum expansion capped at 0.9 quantile: Inf
.
I'm not sure how to interpret that.
glue::glue("{species_code}_acomps_{max(doabins)}.csv") | ||
), | ||
abins = 0:max(doabins), | ||
sum1 = TRUE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got
Warning message:
The `sum1` argument of `writeComps()` is deprecated as of PacFIN.Utilities 0.2.10.
strat = c("state", "geargroup"), | ||
# TODO: Determine if we want ROUND_WEIGHT_LBS | ||
valuename = "LANDED_WEIGHT_LBS" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For most species round weight and landed weight are equal, but generally one should always used round weight. Sablefish is one of the species where this matters. Vessels often remove sablefish heads at-sea (or do some form of dressing reducing fish weight) and the round weight column is based on the landed weight of the partial fish multiplied by an expansion factor to estimate the weight of the whole fish.
Question - why are we using the pounds column? I typically use the ROUND_WEIGHT_MTONS column to avoid having to do additional calculations. Since the landed weight is used in the second stage expansion, scaling up samples by relative landed weight by gear and state, the units likely don't matter but I would have expected metric tons here. Have I been doing it wrong?
I would also suggest adding text that one could read in pre-processed catches stratified by state and gear group here if the user has modified the PacFIN landings in some way.
@iantaylor-NOAA do you have time to test out this branch for me. Right now this is just a draft PR. I need to look at the final input sample size and ensure that I am passing the correct column but other than that it should be good.