Remote GAMBLR (outside GSC) is now a thing. Testers wanted! #99

rdmorin · 2022-06-02T17:16:19Z

This was tested on my laptop using the test_remote.R script bundled with GAMBLR. The snakefile to allow the user to sync the required files is also provided. I haven't made a yml file for that environment yet.

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)

…and capture

rdmorin · 2022-06-08T00:52:33Z

This now has a patch for cBIoportal (portal.R) that fixes some issues caused by recent updates.

…tion is set up right

Kdreval · 2022-06-10T23:18:16Z

R/checks.R

+      missing = required_cols[!required_cols %in% colnames(metadata_df)]
+      message("FAIL! MISSING metadata columns:")
+      print(paste(missing,sep=" "))
+      return()


What about adding here else part that will just say something alongside "Success! All required columns are present in the metadata". This way we are letting user know that the completeness check was performed and all is well

Kdreval · 2022-06-10T23:32:51Z

R/viz.R

+                      "pathology" = "pathology", "analysis_cohort" = "pathology", "group" = "pathology",
+                      "FL_group" = "FL", "lymphgen" = "lymphgen", "lymphgen_with_cnv" = "lymphgen",
+                      "bcl2_ba" = "pos_neg", "BCL2_status" = "pos_neg", "myc_ba" = "pos_neg",
+                      "bcl6_ba" = "pos_neg", "manta_BCL2_sv" = "pos_neg", "manual_BCL2_sv" = "pos_neg", "manta_MYC_sv" = "pos_neg")


One other thing. I can think of adding here is "EBV_status_inf"="EBV_status"

Kdreval · 2022-06-10T23:35:25Z

data-raw/DATASET.R

+lymphoma_genes = left_join(lymphoma_genes,gene_detail) 
+lymphoma_genes$LymphGen=FALSE
+lymphoma_genes[lymphoma_genes$entrezgene_id %in% lymphgen_anno$NCBI_Gene_ID,"LymphGen"] = TRUE
+


It seems that here line
usethis::use_data(lymphoma_genes, overwrite = TRUE)
might be missing?

Kdreval · 2022-06-10T23:37:58Z

get_gambl_results.smk

+    output:
+        sv_combined
+    run:
+        shell("cp {input[0]} {output[0]}")


New line is missing here, but would it make sense to also add the expression data syncing here?

mattssca

This works as intended on Rstudio (remote, gphost) for me. Please see the comments in the review for a few questions and thoughts.

I will give this a go on my local machine as well (using snakefile to get all required files).

As for cBioPortal study setup, importing the genome study works fine (currently added to our instance), but for some strange reason (Docker being Docker...), it does not work for the capture study. I will troubleshoot this and it's very likely that this error is related to Docker and not the code in this PR. So should be fine to approve this PR.

edit: The capture study was successfully imported into our cBioPortal instance. This was achieved by increasing the memory allocated to Docker.

mattssca · 2022-06-13T20:11:39Z

test_remote.R

+# all these should proceed without error
+coding_ssm = get_coding_ssm(these_samples_metadata = all_meta,seq_type = "genome")
+
+cap_ssm = get_coding_ssm(these_samples_metadata = cap_meta,seq_type = "capture",seq_type_priority="genome")


This gives an unused argument error:
> cap_ssm = get_coding_ssm(these_samples_metadata = cap_meta,seq_type = "capture",seq_type_priority="genome") Error in get_coding_ssm(these_samples_metadata = cap_meta, seq_type = "capture", : unused argument (seq_type_priority = "genome")
Works with the last argument dropped.

mattssca · 2022-06-13T22:39:11Z

test_remote.R

+
+cbio_path = paste0(config::get("project_base"),"cbioportal-docker-compose/study/GAMBL_capture_2022/")
+
+samples_included = GAMBLR::setup_study(seq_type = "capture",


should seq_type be the same parameter as for genome? (i.e seq_type_filter)

mattssca · 2022-06-13T22:39:40Z

test_remote.R

+                    description = "GAMBL capture data",
+                    out_dir = cbio_path)
+
+finalize_study(seq_type="capture",short_name="GAMBL_capture_2022",sample_ids=samples_included,


same comment as above related to seq_type parameter.

lkhilton

I ran into some errors when running test_remote.R outlined in my comments. I also committed a couple of changes I caught in the process of testing.

lkhilton · 2022-06-14T06:17:06Z

test_remote.R

+svar_all = get_combined_sv(min_vaf = 0.1,projection = "grch37")
+
+dim(svar_all)
+# [1] 450641     19


This returned a very different value for me:

[1] 150162 19

I get 450641 on the gphost too. Must be a truncated file on your end.

lkhilton · 2022-06-14T06:18:30Z

test_remote.R

+
+all_seg = get_sample_cn_segments(sample_list = all_meta$sample_id,multiple_samples = T)
+#dim(all_seg)
+#[1] 375247      7


This returned the following:

[1] 376588 7

Yeah I get that number now too. Maybe that file was updated recently?

lkhilton · 2022-06-14T06:33:11Z

test_remote.R

+cn_seg = get_sample_cn_segments("14-24534_tumorB",from_flatfile = T)
+
+#should automagically get the single seg file and MAF for the sample
+cn_ssm = assign_cn_to_ssm("14-24534_tumorB",seg_file_source = "battenberg",ssh_session = session)


This gives me the following error:

trying to find output from: battenberg looking for flatfile: /projects/nhl_meta_analysis_scratch/gambl/results_local/ ~/gambl/results_local/ Error in file(con, "rb") : cannot open the connection In addition: Warning messages: 1: In file(con, "rb") : 'raw = FALSE' but '/Users/laurahilton/gambl/results_local' is not a regular file 2: In file(con, "rb") : cannot open file '/Users/laurahilton/gambl/results_local': it is a directory

lkhilton · 2022-06-14T06:39:33Z

test_remote.R

+# advanced functions
+
+
+coding_ssm_status = get_coding_ssm_status(gene_symbols = c("EZH2","CREBBP","KMT2D"),these_samples_metadata = cap_meta,maf_data = cap_ssm)


This gives me a similar error message so it's not related to the ssh connection?

Joining, by = "sample_id" Error in file(con, "rb") : cannot open the connection In addition: Warning messages: 1: In file(con, "rb") : 'raw = FALSE' but '/Users/laurahilton/gambl' is not a regular file 2: In file(con, "rb") : cannot open file '/Users/laurahilton/gambl': it is a directory

This function is probably broken because it's not finding the file it needs under your gambl repo. I think we need to start submitting PR and reviewing gambl/GAMBLR updates in sync but I'm not sure the best way to coordinate this. The new files are under the directory you suggested (versioned_results).

lkhilton · 2022-06-14T06:40:33Z

test_remote.R

+
+
+
+# test if we can make a cBioportal instance


I deleted Docker from my laptop today so I'm unhelpful here

No worries. This part has been tested by Adam.

lkhilton · 2022-06-14T21:32:43Z

R/utilities.R

 #'
 #' @examples
 get_ssh_session = function(host="gphost01.bcgsc.ca"){
-  session = ssh_connect(host=host)
+  if (!requireNamespace("ssh", quietly = TRUE)) {


I added library(ssh) to the top of the test_remote.R script because without it in NAMESPACE this throws an error. Is it possible to also add a step to load the library if it's already installed?

Good idea. I suppose we could have it load automatically but I think it's frowned upon to put library() or require() into functions like that.

Since this function only needs to be run once per "session" I figured this warning would be fine

The error message I got was a bit cryptic:

Error in ssh_exec_internal(ssh_session, command = paste("stat", aug_maf_path), : could not find function "ssh_exec_internal"

What is the best practice in this context when we can't have this package listed in DESCRIPTION as an import? Should we just check if the package is loaded with if(!require("ssh")) and throw an error message if that returns TRUE?

Oh. It's not printing this as a message?
"The ssh package must be installed to use this functionality"

rdmorin and others added 7 commits June 1, 2022 11:35

update the gene list, drop some dead genes

abd3808

update gene list and add new gene lists for comparison

7fafe00

updates for functionality outside the GSC

eac56f2

merge with remote

3ef77ae

update gene lists and fix snakefile

4828e80

updates to fix broken functionality and add compatability for genome …

5b5249c

…and capture

update test script

a2abc42

rdmorin requested a review from mattssca June 8, 2022 00:52

rdmorin added 5 commits June 8, 2022 08:13

remove require statements that should be unnecessary if the documenta…

da770e4

…tion is set up right

add new function for finding common issues in the metadata

d5ae680

minor update

f7fc5ce

update and fix bug in expression function

d20954f

bug fix to allow estimate_purity to estimate purity using augmented MAF

41248d7

Kdreval approved these changes Jun 10, 2022

View reviewed changes

Your Name and others added 11 commits June 10, 2022 16:42

add some more remote functionality using on-the-fly scp

e57c195

Merge branch 'rmorin-dev' of github.com:morinlab/GAMBLR into rmorin-dev

199f039

minor tweaks

354d898

updates for working over ssh

b94edbb

fix grepl error in multiple places

8496592

fix function to work with local file when available

3312a9c

fix some more remote dependencies

2c6c0b8

fix some more remote dependencies

8e65499

fix merge conflict

0f6642b

add environment for snakefile

0ccfed1

update to use environment variables

19bb290

mattssca approved these changes Jun 13, 2022

View reviewed changes

lkhilton added 3 commits June 13, 2022 22:40

Add envvars key

16b2f96

Add ssh package to Imports

ab2e57b

Add library(tidyerse)

16ecc30

lkhilton reviewed Jun 14, 2022

View reviewed changes

rdmorin and others added 7 commits June 14, 2022 07:21

add missing argument

8408d79

Merge branch 'rmorin-dev' of github.com:morinlab/GAMBLR into rmorin-dev

14041c3

remove ssh dependency so GAMBLR can install on GSC hosts

1266e75

add expression-getting functionality

f30590c

Add sentinels

51519db

Merge branch 'rmorin-dev' of github.com:morinlab/GAMBLR into rmorin-dev

209ede6

Load ssh library

385b5db

lkhilton reviewed Jun 14, 2022

View reviewed changes

rdmorin and others added 5 commits June 15, 2022 05:52

add functionality for lymphgen

e511cce

merge

4b0a59d

fix lymphgen syncing

98e264e

add function to check for file completeness

d6bf893

add function to check for file completeness

f385ff3

mattssca mentioned this pull request Jun 15, 2022

Cmattsson dev #91

Merged

9 tasks

rdmorin and others added 14 commits June 15, 2022 15:00

fix bug in check function

a90e058

add expression to all rule

cdc09d5

update lympgenerator

0558f24

change defaults for code editing

cfd50d5

new check for out of date files

f7d97e0

add archive directory

9e6f150

update config

a42e184

more updates to check.R

2cf456b

helper function for getting wildcards

4699d5c

some updates to checks.R

4cb519e

Merge branch 'rmorin-dev' of github.com:morinlab/GAMBLR into rmorin-dev

5b8222f

some more updates to enhance remote functionality

c954766

fix another function that relies on get_gambl_metadata

09959cf

fix get_ssm_by_regions

a6b7670

mattssca merged commit 49b509d into master Jun 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote GAMBLR (outside GSC) is now a thing. Testers wanted! #99

Remote GAMBLR (outside GSC) is now a thing. Testers wanted! #99

rdmorin commented Jun 2, 2022

rdmorin commented Jun 8, 2022

Kdreval Jun 10, 2022

Kdreval Jun 10, 2022

Kdreval Jun 10, 2022

Kdreval Jun 10, 2022

mattssca left a comment •

edited

Loading

mattssca Jun 13, 2022

mattssca Jun 13, 2022

mattssca Jun 13, 2022

lkhilton left a comment

lkhilton Jun 14, 2022

rdmorin Jun 14, 2022

lkhilton Jun 14, 2022

rdmorin Jun 14, 2022 •

edited

Loading

lkhilton Jun 14, 2022

lkhilton Jun 14, 2022

rdmorin Jun 14, 2022

lkhilton Jun 14, 2022

rdmorin Jun 14, 2022

lkhilton Jun 14, 2022

rdmorin Jun 14, 2022

rdmorin Jun 14, 2022

lkhilton Jun 14, 2022

rdmorin Jun 14, 2022


		cbio_path = paste0(config::get("project_base"),"cbioportal-docker-compose/study/GAMBL_capture_2022/")

		samples_included = GAMBLR::setup_study(seq_type = "capture",

		# advanced functions


		coding_ssm_status = get_coding_ssm_status(gene_symbols = c("EZH2","CREBBP","KMT2D"),these_samples_metadata = cap_meta,maf_data = cap_ssm)

Remote GAMBLR (outside GSC) is now a thing. Testers wanted! #99

Remote GAMBLR (outside GSC) is now a thing. Testers wanted! #99

Conversation

rdmorin commented Jun 2, 2022

Pull Request Checklists

Checklist for all PRs

Required

Optional but preferred with PRs

Checklist for New Functions

Required

Checklist for changes to existing code

rdmorin commented Jun 8, 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

mattssca left a comment • edited Loading

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

lkhilton left a 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 Jun 14, 2022 • edited Loading

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

mattssca left a comment •

edited

Loading

rdmorin Jun 14, 2022 •

edited

Loading