-
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
Rmorin dev #45
Rmorin dev #45
Conversation
This adds a new function that depends on another (very subtly) forked github repo. It should only break for people if they try to use that new function. |
native_blacklist_path = paste0(config::get("project_base"),unix_group,"/",tool_name,"-",tool_version,"_",annotator_name,"-",annotator_version,"/level_3/variants_",genome_build,"_native_clean_blacklist.txt") | ||
lifted_blacklist_path = paste0(config::get("project_base"),unix_group,"/",tool_name,"-",tool_version,"_",annotator_name,"-",annotator_version,"/level_3/variants_",genome_build, "_lifted_clean_blacklist.txt") | ||
}else{ | ||
annotator_version="1.2" |
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.
Just want to make sure: the annotator_version
is conditional in the clustered flavour (so can be modified by user and is not necessarily 1.2), but here it is always set to 1.2, even if the user specifies say 1.1 when calling this function?
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.
This was somewhat purposeful. It's mostly just providing a slightly more standardized way to construct the paths for the different file sets. I don't anticipate (or handle) other options for now.
# Merge the CN info to the corresponding MAF file, uses GAMBLR function | ||
if(!missing(in_seg)){ | ||
if(missing(in_maf) & missing(in_seg)){ | ||
CN_new <- assign_cn_to_ssm(this_sample = sample_id,coding_only=coding_only,assume_diploid=assume_diploid,genes=genes)$maf |
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 this break in genes
is missing, or maybe returns an empty object? I think this call here will supply an empty list, and the function assign_cn_to_ssm
will get the genes
argument of empty list, and might filter towards it returning 0 rows. Or is it disregarded when empty?
#' @export | ||
#' | ||
#' @examples | ||
get_mutation_frequency_bin_matrix = function(regions, |
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.
This I think is just moved from utilities
and otherwise is not modified?
geom_tile(data=windows_tallied,aes(x=window_start,y=sample_id,fill=n)) + | ||
scale_fill_gradient(low = "orange", high = "red", na.value = NA) + | ||
theme_cowplot() + scale_colour_manual(values=c(lg_cols,path_cols)) + | ||
theme(axis.text.y=element_blank()) | ||
} | ||
if(plot_type != "none"){ | ||
print(p) |
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.
just noticed it - can we return here instead of print? This way, plot will be accessible as object and then can be modified using any ggplot +
statements
these_samples_metadata = these_samples_metadata %>% dplyr::rename("patient_id"=patient_colname) | ||
both_vaf_all = full_join(t1_pair_mafdat,t2_pair_mafdat,by=c("patient_id","Start_Position")) %>% | ||
dplyr::select(patient_id,HGVSp_Short.x,Hugo_Symbol.x,Hugo_Symbol.y,Tumor_Sample_Barcode.x, | ||
Tumor_Sample_Barcode.y,HGVSp_Short.y,vaf.x,vaf.y,hot_spot.x,hot_spot.y) %>% |
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 think both the maf1 and maf2 should be annotated here - should we add a stop message, or handle it automatically within this function? I can take care of that in my next PR
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)