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

Draft PR: SNV Caller Comparison Notebook #159

Closed
wants to merge 129 commits into from

Conversation

cansavvy
Copy link
Collaborator

@cansavvy cansavvy commented Oct 18, 2019

Purpose/implementation

After my initial series of PRs with scripts to assess snv callers individually, this PR adds the notebook which takes that initial analysis and compares the snv callers to each other.

The eventual goal is to decide on a set of mutations from these callers that we can move forward with.

Two things to keep in mind about the current notebook/PR:

  1. This is only the subset of the data being shown currently, so it will have a lot more data later. This will affect aesthetics of the plots as well as memory requirements to run it.
  2. I haven't written up results/conclusion yet since we are still missing data and I have only run this on a subset of the data.

Issue

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

Directions for reviewers

I'm looking for an initial "broad strokes feedback" about the plots and analyses:
You can see the output notebook here to evaluate my initial questions: https://cansavvy.github.io/openpbta-notebook-concept/snv-callers/compare_snv_callers.nb.html

Some questions for this PR:
Which plots are useful? Which are not?
What do you think of the current analyses?
Are there other analyses you would like to see added?
How do you feel about the plots' aesthetics?

Results

See the rough draft of this notebook output from this notebook here: https://cansavvy.github.io/openpbta-notebook-concept/snv-callers/compare_snv_callers.nb.html

Preliminary conclusion:
From just the subset data it looks like VarDict should be dropped and we could move forward with mutations that are found in Mutect2, Strelka2, and Lancet.

But, we should see how this looks after the v6 data come out and the rest of the samples are added. As of now, we do not have WXS samples for Lancet or VarDict.

Docker and continuous integration

  • The dependencies required to run the code in this pull request have been added to the project Dockerfile.
    Main RPackages added to the Dockerfile:
  • UpSetR
  • GGally
  • ggupset
  • This analysis has been added to continuous integration.
  • Run a linter
  • Set the seed- I didn't, but let me know if I should.
  • Comments and/or documentation up to date - I will add more comments but after we figure out which plots we are keeping
  • Double check your paths
  • Spell check any Rmd file or md file
  • Restart R and run all notebooks fresh and save

@cansavvy
Copy link
Collaborator Author

cansavvy commented Oct 23, 2019

Two Updates:

I made the changes to the version of the original notebook:
https://cansavvy.github.io/openpbta-notebook-concept/snv-callers/compare_snv_callers.nb.html

Here's a rough draft of the VAF cutoff experiment. Let me know what other plots I should include. I think for it's full-fledged review I will make it its own PR.
https://cansavvy.github.io/openpbta-notebook-concept/snv-callers/vaf_cutoff_experiment.nb.html

Let me know what you think, @jashapiro

@jaclyn-taroni
Copy link
Member

This looks like it might be at the stage where it's good to split this up into multiple pull requests to make it easier to review. The VAF filter experiment and comparison of callers seem like natural parts to break up and perhaps the small changes here and there can be split up in some logical way. What do you say @cansavvy and @jashapiro ?

@cansavvy
Copy link
Collaborator Author

cansavvy commented Oct 25, 2019

This looks like it might be at the stage where it's good to split this up into multiple pull requests to make it easier to review. The VAF filter experiment and comparison of callers seem like natural parts to break up and perhaps the small changes here and there can be split up in some logical way. What do you say @cansavvy and @jashapiro ?

Yes this is what my plan for today has been. Should have noted that on here. This PR grew a bit out of control.

@jaclyn-taroni
Copy link
Member

When breaking it up, I would consider what smaller changes would not necessarily benefit from considerable background knowledge about what you've done so far @cansavvy. @jashapiro is a good person to review what steps would really benefit from that prior knowledge. Let's spread review of other things across multiple reviewers.

@cansavvy
Copy link
Collaborator Author

cansavvy commented Oct 25, 2019

I think I will go ahead and close this PR because its served its purpose but is replaced by #172, #173 and #174

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