search

pfmc-assessments / PacFIN.Utilities

R code to manipulate data from the PacFIN database for assessments
http://pfmc-assessments.github.io/PacFIN.Utilities
Other
7 stars 1 forks source link

Lengthage kfj #25

Closed kellijohnson-NOAA closed 3 years ago

kellijohnson-NOAA commented 3 years ago

@k-doering-NOAA and @brianlangseth-NOAA thank you for getting Ian's function into PacFIN.Utilities and the improvements you made. @iantaylor-NOAA thank you for starting this function. Everyone's hard work is really nice to see in git rather than on individual machines.

I made some changes to the function, where most will probably go unnoticed by users, except the new ability to click through individual plots if you don't want to save them all to a disk or you just want to look at a few larger ones. This can now be done with wait2plot = TRUE which is not the default.

Perhaps others can review my pull request before bringing it into lengthage, then after merging we can all collectively look at the additional things that I noted in the last commit that still could be done. Then @chantelwetzel-noaa could move this function to its new location :)

k-doering-NOAA commented 3 years ago

Thanks @kellijohnson-NOAA ! I was thinking about trying to generalized to bds or survey data (and updating the example) and am glad to see you have beat me to it! I looked at the changes you made and they look good. I don't have time to test out the function interactively right now, but can do that next week; however, @brianlangseth-NOAA if you are able to take a look, feel free to merge in the changes as you wish.

kellijohnson-NOAA commented 3 years ago

Thanks @k-doering-NOAA Let's wait until @iantaylor-NOAA pushes his fixes ... that can be at least one test b/c I didn't formally write any (shame shame).

brianlangseth-NOAA commented 3 years ago

Thank @kellijohnson-NOAA . Im continually impressed by the skill in our group.

I noticed a few adjustments that I can add in if you prefer. Not sure of the process. The check on max_break should probably be > as opposed to >=. Hard to do max(length) + 1 in the call. Also, if file is provided then wait2plot doesn't do anything. Maybe this is by design but perhaps changing documentation would make it more clear?

Also, bhat was meant to be bhat. (as short for bhattacharyya). I can replace with beta and will shorten the KS pvalue too.

Lastly, I can update documentation with the @details.

iantaylor-NOAA commented 3 years ago

Looks good to me. The click through images doesn't work for me, but writing to file or using windows(record = TRUE) before running the function allows me to see the individual panels.

I made a few minor changes in the recent commits.

kellijohnson-NOAA commented 3 years ago

sorry @brianlangseth-NOAA for butchering the bhat

Should I remove the wait given it doesn't work for everyone?

iantaylor-NOAA commented 3 years ago

I don't know why the hat doesn't work for everyone but I would go back to Bhat.

I think the amount of time and effort that has gone into this function is out of proportion to the expected amount of use it will get (although it was useful for me testing it just now to see that commercial Big Skate ages from 2018 were from disproportionately larger fish, supporting the choice we made in that assessment to use conditional age-at-length data).

However, I would hope that the processes used here (including contributions and testing by multiple folks and things like a pull requests from @kellijohnson-NOAA and a code review from @k-doering-NOAA could serve as a model for work on other shared tools in nwfsc-assess).

kellijohnson-NOAA commented 3 years ago

@iantaylor-NOAA I meant that the clickable figures wasn't working for you

iantaylor-NOAA commented 3 years ago

Sorry, I just didn't get that I had to add wait2plot = TRUE. Works just fine.