-
Notifications
You must be signed in to change notification settings - Fork 0
add strand flip smart check #82
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
|
Hey @jarbet I don't think I have access to request Copilot for a review. Could you give it a go again? |
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 introduces a new max.strand.flips option to condition ambiguous strand flip handling on the count of unambiguous flips, extends it through apply.polygenic.score, updates documentation and vignettes, and adds corresponding unit tests.
- Add
max.strand.flipsargument toassess.pgs.vcf.allele.matchandapply.polygenic.score - Update vignette and man pages to document new behavior
- Introduce unit tests covering threshold logic in both allele matching and PGS application
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vignettes/UserGuide.Rmd | Added max.strand.flips and custom pgs.columns notes; updated plotting list |
| tests/testthat/test-strand-flip-handling.R | New tests for ambiguous flip threshold behavior |
| tests/testthat/test-pgs-application.R | New end-to-end tests for max.strand.flips in PGS application |
| man/assess.pgs.vcf.allele.match.Rd | Documented max.strand.flips parameter |
| man/apply.polygenic.score.Rd | Documented max.strand.flips parameter |
| R/assess-strand-flip.R | Implemented threshold logic and roxygen updates |
| R/apply-pgs.R | Pass and adjust max.strand.flips through main flow |
| NEWS.md | Added release note for conditional strand flip removal |
Comments suppressed due to low confidence (2)
man/apply.polygenic.score.Rd:43
- Update the documentation to reference the correct high-level argument name (
remove.ambiguous.allele.matches) and wrap the condition in \code{} instead of braces.
\item{max.strand.flips}{An integer indicating the number of unambiguous strand flips that need to be detected in order to discard all variants with ambiguous allele matches. Only applies if {return.ambiguous.as.missing == TRUE}.
R/assess-strand-flip.R:82
- Wrap parameter references in \code{} (e.g., \code{return.ambiguous.as.missing == TRUE}) and correct the spelling of "ambigous_flip" to "ambiguous_flip" in the roxygen comment.
#' @param max.strand.flips An integer indicating the number of non-ambiguous strand flips that must be present to implement the discarding all allele matches labeled "ambigous_flip". Only applies if {return.ambiguous.as.missing == TRUE}.
In this PR:
assess.pgs.vcf.allele.matchthat conditions ambiguous strand flip removal on the number of detected unambiguous allele matches.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