New plotting functions and various improvements. #110

mattssca · 2022-07-04T20:54:27Z

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)

mattssca · 2022-07-04T20:55:01Z

More chances to come, will notify you when PR is ready for review. Thanks,

rdmorin · 2022-07-06T19:41:43Z

R/viz.R

-                            use_augmented_maf = TRUE){
+                            use_augmented_maf = TRUE,
+                            add_qc_metric = FALSE,
+                            seq_type = "genome"){


You added seq_type but didn't modify the place where functions are called that would require this parameter to make it useful, e.g. the call to assign_cn_to_ssm

This parameter was added to satisfy the call of collate_qc_results. But I should also update the call of assign_cn_to_ssm to use the same seq_type. Thanks!

rdmorin · 2022-07-06T19:46:13Z

R/viz.R

-      as.data.frame()
+  #get gambl metadata (if not supplied)
+  if(missing(metadata)){
+    this_meta = get_gambl_metadata()


This will default to genome. You must past the seq_type along (seq_type_filter=seq_type). EVERY line that calls get_gambl_metadata MUST have this explicit and passed from the calling function.

Indeed, thanks for your comment. All metadata functions used in these plotting functions will be updated to call seq_type_filter.

rdmorin · 2022-07-06T19:50:19Z

R/viz.R

-      arrange(sample_id) %>%
-      as.data.frame()
+  #filter metadata on selected cohort (if filter_cohort is called)
+  if(missing(these_samples)){


This will not work as expected if someone supplies BOTH filter_cohort AND filter_pathology, will it?

No, it will not. The way I designed this function was so that the user can define one or the other. But I can see how it could be useful to further subset your data based on both conditions. I will address this.

rdmorin · 2022-07-06T19:52:57Z

R/viz.R

+    }else if(!missing(filter_pathology)){
+      these_samples = dplyr::filter(this_meta, pathology == filter_pathology) %>%
+        dplyr::select(sample_id) %>%
+        as.data.frame()


This really should not be if/else it should just be individual if for any filter. I also notice you switched between if(!missing) and if(!not.null) and only one of those is likely to work

rdmorin · 2022-07-06T19:54:13Z

R/viz.R

-    these_samples = dplyr::filter(this_meta, pathology == filter_pathology) %>%
-      dplyr::select(sample_id) %>%
-      as.data.frame()
+  if(missing(these_samples)){


Same issue as above. Is the idea here that someone will only ever be expected to filter using one or the other but never combine them? If so, then you will need to check that they never provide more than one filter and throw an error if they do

Yes, see my response to your previous comment. It's the same for all these functions. I think it's best to have the function filter on both if both parameters are used.

rdmorin · 2022-07-06T19:55:44Z

R/viz.R

+
+  #get gambl metadata (if not supplied)
+  if(missing(metadata)){
+    this_meta = get_gambl_metadata()


Same issue I raised previously. NEVER call this function in another function without passing it seq_type_filter from the calling function

Noted, the full collection of these fancy QC plots needs to be updated in this regard.

rdmorin

Some major issues here that have me worried you don't fully understand some of the nuances of how get_gambl_metadata works within functions.

mattssca · 2022-07-06T20:01:08Z

Thanks for your comments Ryan. I completely understand the importance of the issues you raised and also that it is important to call get_gambl_metadata with the correct parameters. I am continuously making updates to these functions over the last couple of days and as the comment on this PR states, It is not 100% for review yet. All good comments though, I will address these issues in an upcoming commit. Please do not worry.

…s, (3)hot-fixes, parameter updates, updating if statements etc.

… and other small fixes

mattssca added 2 commits July 4, 2022 13:53

adding new plotting function for proportional coverage (QC)

8220bff

updating package documentation

57a959f

mattssca added 4 commits July 5, 2022 11:56

updates and tweaks to QC plots

1b652d6

updating documentation

18513ed

adding qc metric support to fancy_v_chrcount plot

2e26641

updating documentation

40c08d5

rdmorin reviewed Jul 6, 2022

View reviewed changes

rdmorin requested changes Jul 6, 2022

View reviewed changes

mattssca added 13 commits July 6, 2022 15:37

(1)Adding new QC proportions plot, (2)updates based on review comment…

7097491

…s, (3)hot-fixes, parameter updates, updating if statements etc.

updating package documentation

baef8f2

updating NAMESPACE

3e63aa6

updates to fancy_proportions_plot

87a526d

updates to fancy_qc_plots

a584606

adding new vignette (fancy_x_plots)

be62bdd

vignette updates

7cd6941

adding support for when no sample_table is provided + parameter usage…

a7f46b2

… and other small fixes

updating documentation

9d2bb75

updating fancy_v_count colours (SNPs)

a1d078c

update documentation

9d1c945

updating fancy_v_count plot in vignette

69e63a5

Merge branch 'master' into cmattsson-dev

681d980

Kdreval approved these changes Jul 12, 2022

View reviewed changes

rdmorin approved these changes Jul 12, 2022

View reviewed changes

mattssca merged commit fa72181 into master Jul 12, 2022

rdmorin deleted the cmattsson-dev branch July 12, 2022 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New plotting functions and various improvements. #110

New plotting functions and various improvements. #110

mattssca commented Jul 4, 2022 •

edited

Loading

mattssca commented Jul 4, 2022

rdmorin Jul 6, 2022

mattssca Jul 6, 2022

rdmorin Jul 6, 2022

mattssca Jul 6, 2022

rdmorin Jul 6, 2022

mattssca Jul 6, 2022

rdmorin Jul 6, 2022

rdmorin Jul 6, 2022

mattssca Jul 6, 2022

rdmorin Jul 6, 2022

mattssca Jul 6, 2022

rdmorin left a comment

mattssca commented Jul 6, 2022

New plotting functions and various improvements. #110

New plotting functions and various improvements. #110

Conversation

mattssca commented Jul 4, 2022 • edited Loading

Pull Request Checklists

Checklist for all PRs

Required

Optional but preferred with PRs

Checklist for New Functions

Required

Checklist for changes to existing code

mattssca commented Jul 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdmorin left a comment

Choose a reason for hiding this comment

mattssca commented Jul 6, 2022

mattssca commented Jul 4, 2022 •

edited

Loading