-
Notifications
You must be signed in to change notification settings - Fork 0
Add boxplot plotting function #80
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
Conversation
|
A note regarding the R-CMD-check: In my case it was an unclosed parenthesis in a new example for custom PGS colnames. |
|
@jarbet @rhughwhite |
rhughwhite
left a comment
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.
Looks good to me! Also, you could try getting an extra review from Copilot? I think you need to enable it in the repo settings.
| } | ||
| } else { | ||
| # If no pgs columns provided, default to recognized PGS columns | ||
| recognized.pgs.colnames <- c('PGS', 'PGS.with.replaced.missing', 'PGS.with.normalized.missing'); |
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.
You could potentially just define the default columns once using a helper function and call it when needed.
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.
true, but i don't feel like rewriting this in 4 plotting functions right now
| } else { | ||
| # If no pgs columns provided, default to recognized PGS columns | ||
| recognized.pgs.colnames <- c('PGS', 'PGS.with.replaced.missing', 'PGS.with.normalized.missing'); | ||
| pgs.columns <- colnames(pgs.data)[colnames(pgs.data) %in% recognized.pgs.colnames]; |
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.
Not sure if it's needed but could check that the user input columns contain numeric data?
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.
Good call! I'll add
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.
Pull Request Overview
This PR adds a new boxplot plotting function and enhances existing plotting functions to support user-provided PGS column names. The changes include:
- Added new
create.pgs.boxplot()function that creates boxplots for PGS distributions with optional grouping by categorical phenotypes - Enhanced
create.pgs.density.plot()andcreate.pgs.with.continuous.phenotype.plot()to accept custom PGS column names via thepgs.columnsparameter - Added comprehensive unit tests for the new functionality
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| R/plot-pgs.R | Adds new create.pgs.boxplot() function and updates existing plotting functions to support custom PGS column names |
| tests/testthat/test-plotting.R | Adds comprehensive unit tests for the new boxplot function and custom PGS column functionality |
| man/create.pgs.boxplot.Rd | Documentation for the new boxplot function |
| man/create.pgs.density.plot.Rd | Updated documentation with custom PGS column parameter and corrected examples |
| man/create.pgs.with.continuous.phenotype.plot.Rd | Updated documentation with custom PGS column parameter |
| NEWS.md | Records the new functionality in the changelog |
| NAMESPACE | Exports the new boxplot function |
Comments suppressed due to low confidence (1)
tests/testthat/test-plotting.R:561
- The variable name
pgs.boxplots.by.phenotype.plotsis inconsistent with the variablepgs.boxplots.by.phenotypeused elsewhere in the function. This appears to be a typo and should bepgs.boxplots.by.phenotype.
}
In this PR:
create.pgs.boxplot()function that plots the PGS distribution as a boxplot/stripplot, and automatically groups by user-specified categorical phenotypesI have read the code review guidelines and the code review best practice on GitHub check-list.
The name of the branch is meaningful and well formatted following the standards, using [AD_username (or 5 letters of AD if AD is too long)-[brief_description_of_branch].
I have set up or verified the branch protection rule following the github standards before opening this pull request.
I have added the changes included in this pull request to
NEWSunder the next release version or unreleased, and updated the date.I have updated the version number in
metadata.yamlandDESCRIPTION.Both
R CMD buildandR CMD checkrun successfully.Testing Results
All unit tests PASS