Skip to content
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

Jump to bottom

Adding error messages to functions that reads from local files. #117

Merged
merged 62 commits into from
Oct 20, 2022
Merged

Adding error messages to functions that reads from local files. #117

merged 62 commits into from
Oct 20, 2022

Conversation

mattssca
Copy link
Contributor

@mattssca mattssca commented Aug 11, 2022 •
edited
Loading

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.

  • I generated the documentation and checked for errors relating to the new function (e.g. devtools::document()) and added NAMESPACE and all other modified files in the root directory and under man.

Optional but preferred with PRs

  • I updated and/or successfully knitted a vignette that relies on the modified code (which ones?)

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:

#' Use GISTIC2.0 scores output to reproduce maftools::chromoplot with more flexibility
#'
#' @param scores output file scores.gistic from the run of GISTIC2.0
#' @param genes_to_label optional. Provide a data frame of genes to label (if mutated). The first 3 columns must contain chromosome, start, and end coordinates. Another required column must contain gene names and be named `gene`. (truncated for example)
#' @param cutoff optional. Used to determine which regions to color as aberrant. Must be float in the range [0-1]. (truncated for example)
  • My function uses a library that isn't already a dependency of GAMBLR and I made the package aware of this dependency using the function documentation import statment.

Example:

#' @return nothing
#' @export
#' @import tidyverse ggrepel

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)

PR Comments

This PR includes error messages for the following functions;
get_ssm_by_samples
get_ssm_by_sample
get_gambl_metadata
add_icgc_metadata (internal function for get_gambl_metadata)
get_gambl_outcomes (internal function for get_gambl_metadata)
get_combined_sv
get_manta_sv
get_sample_cn_segments
get_coding_ssm
annotate_ssm_blacklist
collate_results
assign_cn_to_ssm
get_ssm_by_region
get_excluded_samples

The above-mentioned functions have all been tested on remote GAMBLR and all return the added error message if Sys.setenv(R_CONFIG_ACTIVE = "remote" has not been specified.

Functions have also been tested after specifying the config to remote, and the collection of the above-mentioned functions runs the intended way, except for; get_combined_sv, get_sample_cn_segments, and get_ssm_by_region. I am currently looking into why the files needed for these functions are not accessible when running GAMBLR remote with a VPN connection.

Other small updates in this PR include typos in the README as well as removing trailing white spaces.

rdmorin
rdmorin reviewed Aug 25, 2022
View reviewed changes
rdmorin
rdmorin reviewed Aug 25, 2022
View reviewed changes
rdmorin
rdmorin reviewed Aug 25, 2022
View reviewed changes
rdmorin
rdmorin reviewed Aug 25, 2022
View reviewed changes
rdmorin
rdmorin reviewed Aug 25, 2022
View reviewed changes
R/utilities.R Outdated
if(! seq_type_filter %in% c("genome", "capture")){
stop("Please provide a valid seq_type (\"genome\" or \"capture\").")
}

#get paths
base = config::get("project_base") #base
qc_template = config::get("qc_met") #qc metric template
qc_path = glue::glue(qc_template) #glue wildcards
icgc_qc_path_full = paste0(base, qc_path) #combine for icgc_dart qc metric path
gambl_qc = gsub("icgc_dart", "gambl", qc_path) #use gsub to substitute icgc_dart in qc path with gambl
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the standard way we substitute in wildcards. You should look at the other examples where we use glue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

rdmorin
rdmorin reviewed Aug 25, 2022
View reviewed changes
rdmorin
rdmorin reviewed Aug 25, 2022
View reviewed changes
R/viz.R Outdated
#filter metadata on selected cohort/pathology
if(missing(these_samples)){
if(missing(these_samples) && missing(these_samples_metadata)){
if(!missing(filter_cohort) && missing(filter_pathology)){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be named "keep_cohort" or something that's more directionally descriptive? I don't know from the name if it means the cohort I want to keep or drop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good comment, makes more sense to name it "keep".

rdmorin
rdmorin requested changes Aug 25, 2022
View reviewed changes
Copy link
Collaborator

@rdmorin rdmorin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left many questions and some comments. Not yet fully reviewed

rdmorin
rdmorin reviewed Aug 25, 2022
View reviewed changes
rdmorin
rdmorin reviewed Aug 25, 2022
View reviewed changes
rdmorin
rdmorin reviewed Aug 25, 2022
View reviewed changes
rdmorin
rdmorin reviewed Aug 25, 2022
View reviewed changes
lkhilton
lkhilton requested changes Oct 6, 2022
View reviewed changes
@rdmorin
Copy link
Collaborator

rdmorin commented Oct 7, 2022

Is this ready for re-review?

@mattssca
Copy link
Contributor Author

mattssca commented Oct 7, 2022

Yes, it's ready.

@rdmorin
Copy link
Collaborator

rdmorin commented Oct 7, 2022

OK. Can you also please respond to my question above?

rdmorin
rdmorin reviewed Oct 20, 2022
View reviewed changes
R/database.R Outdated
@@ -196,23 +191,32 @@ get_ssm_by_samples = function(these_sample_ids,
maf_path = glue::glue(maf_template)
full_maf_path = paste0(config::get("project_base"), maf_path)
message(paste("using existing merge:", full_maf_path))

#check for missingness
if(!file.exists(full_maf_path)){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this code removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, maybe this was an unfortunate case of auto-merge. I'll add it back in.

rdmorin
rdmorin reviewed Oct 20, 2022
View reviewed changes
R/preprocessing_io.R
@@ -733,24 +733,156 @@ find_files_extract_wildcards = function(tool_results_path,

#' Title
#'
#' @param maf_file_path
#' @param maf_file_path path to maf that is about to be read.
#' @param select_cols Optional parameter for specifying what columns are to be returned. If not specified, all columns will be kept.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you describe (is it a numeric vector? Character vector of column names? Either?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed I can, stay tuned for an additional commit with the two requested updates.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry my question was actually about select_cols Optional parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok I see. Let's update that one as well.

rdmorin
rdmorin requested changes Oct 20, 2022
View reviewed changes
Copy link
Collaborator

@rdmorin rdmorin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See individual comments. Just wondering about a few changes

@mattssca
Copy link
Contributor Author

This PR has now been updated based on the latest review comments, in this commit.

@mattssca mattssca requested a review from rdmorin October 20, 2022 20:11
@rdmorin rdmorin merged commit 14a3914 into master Oct 20, 2022
@rdmorin rdmorin deleted the cmattsson-dev branch October 20, 2022 20:43
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@rdmorin rdmorin rdmorin approved these changes

@lkhilton lkhilton lkhilton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mattssca @rdmorin @lkhilton