-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fancy Plots + QoL Updates #115
Conversation
This PR includes updates to a collection of fancy plots. The following revisions have been made.
This PR also includes a few minor QoL updates. Including an update to cnvKompare to always retain the same columns in the returns (regardless if concordant or discordant calls are returned) as well as a hot-fix to collate_results (see issue 112). Before review of this PR, I want to include one more commit I am working on as well as some more thorough testing of the updated plotting functions. I will call for reviewers when the PR is ready to be reviewed |
NAMESPACE
Outdated
@@ -101,6 +101,7 @@ export(tidy_lymphgen) | |||
export(web_initialize_gambl_site) | |||
import(ComplexHeatmap) | |||
import(DBI) | |||
import(GenomicRanges) |
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.
Can you please give an idea on what the GenomicRanges are used for? If it is just to overlap the coordinates, we can consider using the foverlaps and avoid adding another big-sized dependecy
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.
Indeed, the GenomicRanges
package is used for intersecting two bed-like files. I will revert this to using foverlaps
as suggested above. Thanks!
R/viz.R
Outdated
if(intersect_regions){ | ||
#filter CN states on intersecting regions | ||
#transform regions to Granges objects | ||
grl = GRanges(cn_states) |
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.
Alright, see it here now! Can we consider switching this to data.table::foverlaps()
?
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.
Yes, I will do this. Good suggestion!
…versions and adding CN2 as an optional plotting parameter
data.table::setkey(regions_sub, chrom, start, end) | ||
|
||
#intersect regions | ||
intersect = data.table::foverlaps(incoming_cn, regions_sub, nomatch = 0) |
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.
nice addition 😄
Pull Request Checklists
Important: When opening a pull request, keep only the applicable checklist and delete all other sections.
Checklist for all PRs
Required
I tested the new code for my use case (please provide a reproducible example of how you tested the new functionality)
I ensured all dplyr functions that commonly conflict with other packages are fully qualified.
This can be checked and addressed by running
check_functions.pl
and responding to the prompts. Test your code after you do this.devtools::document()
) and addedNAMESPACE
and all other modified files in the root directory and underman
.Optional but preferred with PRs
Checklist for New Functions
Required
I documented my function using ROxygen style.)
All parameters for the function are described in the documentation and the function has a decriptive title.
Example:
import
statment.Example:
Checklist for changes to existing code
I added/removed arguments to a function and updated documentation for all changed/new arguments
I tested the new code for compatability with existing functionality in the Master branch (please provide a reprex of how you tested the original functionality)