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

SNV caller analysis part 1 revamp #172

Merged
merged 138 commits into from
Oct 25, 2019

Conversation

cansavvy
Copy link
Collaborator

@cansavvy cansavvy commented Oct 25, 2019

Purpose/implementation

This PR implements a few smaller changes that were initially discussed and implemented on a retired PR #159.

Changes in this PR:

  1. Makes the regional analysis optional throughout the pipeline.
    • Adds as an optparse option in both 01-calculate_tmb_vaf.R and in 02-run_eval.R
    • Employs the --no_region option in run_caller_evals.sh
  2. Fixes a chromosome labeling problem with the COSMIC file.
    • Chromosome's X and Y were labeled as 23 and 24 though our data has them as X and Y.

Issue

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

Directions for reviewers

Are these changes implemented well?
Does everything seem to be in order?

Is there a better way for me to have the non-regional report as opposed to having a whole other copy of that template? I do not like having the whole thing repeated but am unsure as to an alternative?

Results

No new results from these changes.

Docker and continuous integration

*These changes do not require new packages or new CircleCI tests. *

  • Run a linter
  • Set the seed- I didn't, but let me know if I should.
  • Comments and/or documentation up to date
  • Double check your paths
  • Spell check any Rmd file or md file
  • Restart R and run all notebooks fresh and save

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.

As as making your template flexible, you could break it into pieces. One file with the main section, one file with the region analysis section, then a session info section file. You would then cat them together on the fly. But I don't necessarily think that is worth the effort, especially for this stage of analysis.

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. Just a couple little things.

analyses/snv-callers/README.md Outdated Show resolved Hide resolved
analyses/snv-callers/util/plot_functions.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.

Approved!

@jaclyn-taroni jaclyn-taroni merged commit cee8bb2 into AlexsLemonade:master Oct 25, 2019
@cansavvy cansavvy deleted the snv-caller_revamp branch December 19, 2019 13:36
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