Functionalize CDF TMB Plot #612
Conversation
There was a problem hiding this comment.
@jashapiro has had a heavy review load lately, so figured I would go ahead and review this pull request. The code included in this function has been reviewed before and overall it looks good. There is one thing that is hardcoded and not documented (the minimum number of samples required for display) that should be remedied before merging. Another question I had – how difficult would be it take an argument for how things should be grouped (e.g., not just using short_histology)?
| # Only plot histologies groups with more than `min_samples` number of samples | ||
| dplyr::group_by(short_histology, add = TRUE) %>% | ||
| # Only keep groups with this amount of samples | ||
| dplyr::filter(dplyr::n() > 5) %>% |
There was a problem hiding this comment.
This should be an argument for this function rather than hardcoded and not included in the documentation.
|
I'll add a thing to make this versatile for other fields besides |
|
Let me know how you feel about these changes, and if we are good with them, I'll file another PR that rearranges the chromosomal-instability plot to use this function and then also merge and update these changes with #613 |
Purpose/implementation Section
What scientific question is your analysis addressing?
No new scientific question, just moving the CDF TMB plot to its own file so it is more accessible.
Note that GitHub can't track that the same exact code moved from the notebook to the R script but it is the same exact code.
What was your approach?
Put the CDF plot it its own file so it can be sourced anywhere, Also added more documentation.
What GitHub issue does your pull request address?
Related to this comment and this issue.
Directions for reviewers. Tell potential reviewers what kind of feedback you are soliciting.
Results
No new results.
Reproducibility Checklist
No changes to this.