-
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
add function for gridss svs #47
Conversation
#' @param min_vaf The minimum tumour VAF for a SV to be returned | ||
#' @param min_score The lowest Manta somatic score for a SV to be returned | ||
#' @param with_chr_prefix Prepend all chromosome names with chr (required by some downstream analyses) | ||
#' @param min_vaf |
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.
these 2 params seems to be redundant with the ones above and projection is missing, but I think it can stay like this for now. I can take care of this after marging
dplyr::filter(VAF_tumour >= min_vaf & SOMATIC_SCORE >= min_score) | ||
|
||
if(!missing(this_sample_id)){ | ||
all_sv = all_sv %>% dplyr::filter(tumour_sample_id == sample_id) |
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.
will it work for a list of ids? Should we switch for %in%
here?
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.
Sure. If you do that you should probably rename the parameter these_sample_ids
. I don't really think this parameter will get used much anyway. The user can always apply that filter after calling the function so it's a bit redundant.
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.
Makes sense, I see, thanks!
)) | ||
|
||
} | ||
return(all_sv) |
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.
do you think we can call liftover_bedpe
here to support the hg38 coordinates?
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.
In theory, yes, but it would be super slow on this many records. I would prefer to have this pre-computed on everything so we can just load the merge of either reference instead of any lifting over on-the-fly. Since Laura prepared these files it would be up to her to decide if she wants to write code to lift things in the opposite direction or someone else could extend her code once it's in a PR.
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.
I see, makes sense, for sure. If Laura doesn't have time I can work on this. I think we can merge this as is, and add the hg38 in the future update
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)