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

Add flag for years with large proportions of unsexed fish #50

Open
chantelwetzel-noaa opened this issue Feb 17, 2021 · 11 comments
Open
Assignees
Labels
priority: high The highest priority level in terms of what needs to be done. status: in progress Currently working on this issue topic: code Related to R code within this package type: enhancement
Milestone

Comments

@chantelwetzel-noaa
Copy link
Contributor

I think there should be a flag somewhere if a large proportion of samples from a single year are of unsexed fish. Users may want to treat those data in a different way. I encountered an issue where samples from early years were all for unsexed fish and I was not paying attention. I ran the getComps function and then doSexRatio which split the data to a sex. I spent a whole afternoon trying to sort out why these select years had composition data that looked so strange.

I think a flag would have helped me identify the issue easier. Additionally, it would be nice to have the functionality to not assign a sex via doSexRatio and have the writeComps function spit out unsexed composition data.

@kellijohnson-NOAA kellijohnson-NOAA added type: enhancement status: question Questions about the issue need answered topic: code Related to R code within this package priority: low The lowest level priority, i.e., not urgent. labels May 4, 2022
@kellijohnson-NOAA kellijohnson-NOAA added this to the year_2022 milestone May 4, 2022
@kellijohnson-NOAA kellijohnson-NOAA modified the milestones: year_2022, year_2023 May 8, 2023
@kellijohnson-NOAA
Copy link
Contributor

@chantelwetzel-noaa Thanks for the idea, sorry I moved it continually from milestone to milestone 🤕. I am wondering if we should get rid of doSexRatio() all together. Or at least that is the feeling that I have. I think that unsexed fish should remain unsexed and be put into the model that way rather than making up data.

@kellijohnson-NOAA kellijohnson-NOAA self-assigned this May 10, 2024
@brianlangseth-NOAA
Copy link
Contributor

Some species have few unsexed fish compared to sexed, and vice versa. Although dropping the unsexed fish samples is an option, I dont particularly like that, so Ive used the option to group the unsexed fish into sexed fish knowing that the choice isn't overly impactful on the model and it simplifies the data structure. Thus I am in favor of not getting rid of doSexRatio() but allowing functionality to not assign a sex.

@iantaylor-NOAA
Copy link
Contributor

There's additional discussion about this on this flowchart: https://docs.google.com/presentation/d/1RnfNzP6Nlyp_b4W2yZjbmBrvm5jUe0L8OJxO633dH1k/edit?pli=1#slide=id.g12ab1bd588c_1_5.

I'm convinced that keeping unsexed fish separate or ignoring them (depending on the proportion) are the better ways to go, but I support Brian's wish to give the author the option to partition them into the sexed comps. I don't see that partitioning being fundamentally different than the other expansions that we do.

@chantelwetzel-noaa
Copy link
Contributor Author

After a couple of years of thinking about this, I am supportive of eliminating sex ratio functionality. However, as evidenced by @brianlangseth-NOAA comment above not everyone is on the same page. I think the other option would be to create a universal sex ratio function that users outside of the composition could call functions for both survey and commercial data but with the default functionality of the composition functions to return composition data for all sexes in the data passed to the function. If we go this route I don't think we would need to have a flag so this specific issue could be dismissed.

@iantaylor-NOAA
Copy link
Contributor

@chantelwetzel-noaa, I like your solution, which would also get rid of this problem you described in a comment on the flowchart: "In the survey data, there is an additional level of complexity here. There are 2 option of how to apply the sex ratio 1) apply the sex ratio observed in the same tow to assign sexes or 2) apply the sex ratio across all years observed in the length bin."

However, if we think that partitioning the unsexed fish is going to be phased out over time, is it worth investing the time in building a new function? I'm guessing it would only be worthwhile if doSexRatio() and the equivalent in {nwfscSurvey} require additional maintenance to keep around.

@kellijohnson-NOAA
Copy link
Contributor

I think that the best option via the discussion above is to remove anything related to assigning something a sex if it is unsexed from within PacFIN.Utilities and allow users to assign unsexed fish a sex if they want ahead of time. This could be via some potential future function or user-specific code. The code will always remain in the git history that way if we want to look at it later, it will still be available. I would suggest that a new function, if made, is placed in PEP tools rather than within nwfscSurvey or PacFIN.Utilities.

I plan on HARD deprecating this function doSexRatio() next week. Please let me know as soon as possible with a thumbs down to this comment if this does not work for you.

@brianlangseth-NOAA
Copy link
Contributor

Im struggling to understand all the implication of this. Im going to assume that if doSexRatio() is deprecated then write_comps will be modified to output unsexed fish as that remains the second of the two elements suggested in this issue originally. Thus the main effect would be that there is no current functionality to combine unsexed fish into sexed groups. Based on this discussion, that doesn't happen much, but is it a lot of work to move doSexRatio() into PEPtools as opposed to fully deprecate so that the functionality is maintained?

@kellijohnson-NOAA
Copy link
Contributor

Unsexed fish will be written as unsexed fish. It is not a lot of work to move doSexRatio() to PEPtools but whoever does so would need to fix it to work with datasets other than PacFIN and agree to maintain it there.

@iantaylor-NOAA
Copy link
Contributor

I took a look at all 16 instances of doSexRatio( on github: https://github.com/search?q=doSexRatio%28&type=code.

The three instances where it was not commented out are

  • petrale in 2019 (but it wasn't used in 2023)
  • big skate in 2019 (but only 20 out of 7825 Big Skate in PacFIN were unsexed so should just be ignored in the future)
  • quillback in 2021 (but the reports indicate that in Oregon there were only 46 unsexed fish out of 3071 lengths in Oregon, and in California, almost all the fish were unsexed so there's no enough information to apply doSexRatio() anyway)

There are surely many older assessments not on github that used the function, but if someone wants to reproduce them they would likely have to install an older version of PacFIN.Utilities anyway.

Since @kellijohnson-NOAA does 99% of the work developing and maintaining PacFIN.Utilities, I'm OK going along with whatever she wants. I think that "hard deprecating" means that it won't work. But if someone wants to use it, they can copy the code from PacFIN.Utilities (from git history if necessary) into the code for their assessment and use it or modify it however they wish.

@kellijohnson-NOAA
Copy link
Contributor

Thanks @iantaylor-NOAA for doing that background work. And, yes you are right, I plan on essentially removing the function from the package and giving the user an error saying that it is no longer available. But, yes anyone can always copy the code from the git history.

@kellijohnson-NOAA kellijohnson-NOAA added status: in progress Currently working on this issue priority: high The highest priority level in terms of what needs to be done. and removed status: question Questions about the issue need answered priority: low The lowest level priority, i.e., not urgent. labels May 10, 2024
@iantaylor-NOAA
Copy link
Contributor

To make things extra easy, here's the permanent link to the function prior to deprecation: https://github.com/pfmc-assessments/PacFIN.Utilities/blob/19209f0e8e8aa259d513a0d944df703b3ac4cef6/R/doSexRatio.R.
It might no longer work with future revisions to PacFIN.Utilities, but it's not a long or complex function so probably could be fixed without too much work. At this point we've surely collectively spent more time thinking about unsexed fish than the impact of those fish on the assessments warrant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high The highest priority level in terms of what needs to be done. status: in progress Currently working on this issue topic: code Related to R code within this package type: enhancement
Projects
None yet
Development

No branches or pull requests

4 participants