Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Part 5 of n: SNV caller initial calculations #134

Merged

Conversation

cansavvy
Copy link
Collaborator

@cansavvy cansavvy commented Sep 27, 2019

Purpose/implementation

This is the fifth PR in a series that are preparing a pipeline to perform an initial analysis and calculation of each MAF file for each SNV caller. At this point, the SNV callers that will be evaluated are MuTect2, Strelka2, VarDict, and Lancet.

What these scripts do:
01-calculate_vaf_tmb.R is called from the run_caller_evals.sh bash script and sets up and creates the VAF, TMB, and other files for each dataset based on the options provided. It uses functions from util/wrangle_functions.R and will run the 00-set_up.R script at the very beginning so it makes sure the files needed exist.

These are the files it creates in a results folder for each dataset:

  1. Filtered metadata file
  2. Set up MAF version of the file with VAF
  3. Create an genomic regions annotation file
  4. A TMB file with TMB stats for each sample.

Issue

For SNV caller comparison #103 and Tumor Mutation Burden #3 and sort of #11

Directions for reviewers

Main things I am looking for advice on:

  • How do you feel about the relationship of these scripts in 00-set_up.R? What should be changed/optimized?
  • Where might be some "shoot in the foot" items that should be fixed?
  • How do you feel about the optparse options?
  • As we have accumulated more scripts now, how do you feel about the overall structure and interactions of this analysis? How can it be made less buggy?
  • What do you think about the rprojroot::find_root for getting the directories method?

Results

No results just yet, because we don't yet have all the data.

Docker and continuous integration

  • The dependencies required to run the code in this pull request have been added to the project Dockerfile.
    (From Part 4 of n: SNV Caller Analysis: Set Up Script #126, no changes have been made to the Dockerfile.)
  • This analysis has been added to continuous integration.
  • It is called in the same line as the other SNV set up

PR Checklist

  • Run a linter
  • Set the seed (if applicable)
  • Comments and/or documentation up to date
  • Double check your paths

@cansavvy
Copy link
Collaborator Author

cansavvy commented Oct 1, 2019

The first three datasets passed through the Circle CI check fine, but it has failed because of VarDict. The problems with the VarDict file have been noted. But unfortunately my test won't work until that's resolved.

Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Definitely like the extra file structure. A few comments, mostly minor, including some simplifications for the overwrite logic.

analyses/snv-callers/run_caller_evals.sh Outdated Show resolved Hide resolved
analyses/snv-callers/scripts/00-set_up.R Show resolved Hide resolved
analyses/snv-callers/scripts/01-calculate_vaf_tmb.R Outdated Show resolved Hide resolved
analyses/snv-callers/scripts/01-calculate_vaf_tmb.R Outdated Show resolved Hide resolved
analyses/snv-callers/scripts/01-calculate_vaf_tmb.R Outdated Show resolved Hide resolved
analyses/snv-callers/scripts/01-calculate_vaf_tmb.R Outdated Show resolved Hide resolved
analyses/snv-callers/scripts/01-calculate_vaf_tmb.R Outdated Show resolved Hide resolved
analyses/snv-callers/scripts/01-calculate_vaf_tmb.R Outdated Show resolved Hide resolved
@cansavvy
Copy link
Collaborator Author

cansavvy commented Oct 3, 2019

Okay, made the suggested changes, @jashapiro and things are working fine. What do you think?

Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

Looks good to me, just needs a quick fix on the comments to reflect the recent changes.

analyses/snv-callers/scripts/01-calculate_vaf_tmb.R Outdated Show resolved Hide resolved
Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

👍 Moving on...

@cansavvy
Copy link
Collaborator Author

cansavvy commented Oct 3, 2019

I'm temporarily removing VarDict from the analysis until #135 is unresolved. This way the Circle CI tests will pass and then this can be merged.

VarDict needs to be added back to this analysis after #135 is resolved. This issue is tracked here.

@jaclyn-taroni jaclyn-taroni merged commit 542e3a6 into AlexsLemonade:master Oct 4, 2019
@cansavvy cansavvy deleted the cansav09/snv_calculations branch October 29, 2019 19:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants