document v4 features #88
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the documentation for version 4 features of the ApplyPolygenicScore package, including support for new data formats and plotting functions. The changes focus on making the package more user-friendly and adding comprehensive analysis capabilities.
- Updates major documentation sources (README, UserGuide vignette, manual) to reflect new features
- Introduces new plotting functions and case-control analysis capabilities
- Standardizes parameter naming across functions for consistency
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vignettes/UserGuide.Rmd | Comprehensive update to user guide with new wide/long format explanations, plotting examples, and case-control analysis |
| tests/testthat/test-pgs-statistics.R | Updates test cases to use new pgs.data parameter name |
| man/analyze.pgs.binary.predictiveness.Rd | Updates function documentation to use new pgs.data parameter name |
| README.md | Updates feature list to include new plotting functions and analysis capabilities |
| R/run-pgs-statistics.R | Renames parameter from data to pgs.data for clarity |
| R/plot-pgs.R | Fixes boxplot coloring logic to handle stripplot vs boxplot scenarios |
|
@rhughwhite @jarbet This PR is primarily dedicated to documentation and it's relatively short. I would appreciate a pair of human eyes on it to evaluate readability/comprehension. |
vignettes/UserGuide.Rmd
Outdated
| format the X and Y chromosomes as 'X' and 'Y' respectively, and `numeric.sex.chr = TRUE` to format the X and Y chromosomes as '23' and '24' respectively. | ||
| formatting chromosome names, as these tend to vary between human genome reference GRCh38 and GRCh37 aligned files. | ||
|
|
||
| - Use `chr.prefix = TRUE` to add 'chr' to the chromosome name (GRCh38 style) |
There was a problem hiding this comment.
I think the chr vs no chr is to do with the source/database rather than genome version e.g. USCS = chr, Ensembl = no chr
There was a problem hiding this comment.
huh you're totally right, but for some reason the GRCh37/GRCh38 dichotomy has been very pervasive in my experience too. Maybe every copy of GRCh37 I've ever worked with has been from Ensembl
| pgs.columns = 'PGS.with.replaced.missing', | ||
| phenotype.column = 'continuous.phenotype', | ||
| phenotype.type = 'continuous', | ||
| cutoff.threshold = 0, # binarize at 0 |
There was a problem hiding this comment.
is a default of the median value reasonable/useful?
There was a problem hiding this comment.
Hmm it's a good idea but I think i would rather the user think about this parameter and decide themselves instead of providing a default.
In this PR:
I 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, R CMD Check passes locally