Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reword comments in Mutect2.wdl #5196

Merged
merged 1 commit into from
Oct 1, 2018
Merged

Reword comments in Mutect2.wdl #5196

merged 1 commit into from
Oct 1, 2018

Conversation

takutosato
Copy link
Contributor

In particular, do not call the old orientation bias filter deprecated

@takutosato takutosato changed the title deprecated to recommended Reword comments in Mutect2.wdl Sep 17, 2018
@takutosato
Copy link
Contributor Author

@LeeTL1220 please review

## run_orientation_bias_filter: (deprecated) if true, run the orientation bias filter (optional)
## run_orientation_bias_mixture_model_filter: if true, filter orientation bias sites based on the posterior probabilities computed by the read orientation artifact mixture model (optional)
## run_orientation_bias_filter: if true, run the orientation bias filter (optional)
## run_orientation_bias_mixture_model_filter: if true, filter orientation bias sites with the read orientation artifact mixture model, which is the recommended orientation bias filter (optional)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add a comment about NovaSeq?

Can you add a comment that artifact modes parameter is ignored, since these are automatically detected in the mixture model filter?

Can you add a reference to D-ToxoG for the old filter? https://software.broadinstitute.org/cancer/cga/dtoxog
"This filter is a GATK implementation of D-ToxoG (url) with modifications to allow multiple artifact modes."

Also, we should discuss whether we want to flip the default in FireCloud to be the new filter. Since we are recommending it.

Also, I want to confirm that you have slides with results for the no-artifact case? I.e. that there is no filtering when there is no orientation bias. I want to make sure we have formal results to present before we say it is recommended.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you run both filters at the same time? If not, can you add a comment to that effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All done. The evaluation results are in the palantir repo: https://github.com/broadinstitute/palantir/pull/671

@@ -796,6 +796,7 @@ task CollectF1R2Counts {
}
}

# Learning step of the orientation bias mixture model, which is the recommended orientation bias filter as of September 2017
Copy link
Contributor

Choose a reason for hiding this comment

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

2017? 2018?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops

@@ -737,6 +736,7 @@ task CollectSequencingArtifactMetrics {
}
}

# Data collection step of the orientation bias mixture model, which is the recommended orientation bias filter as of September 2017
Copy link
Contributor

Choose a reason for hiding this comment

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

2017? 2018?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@LeeTL1220
Copy link
Contributor

@takutosato @droazen Can we make sure that this gets in the next release? Users may misinterpret the deprecation warning that is removed in this PR.

@takutosato
Copy link
Contributor Author

@LeeTL1220 thanks for your comments. Yes, I will make suggested changes by the end of the week, if not later today

@takutosato
Copy link
Contributor Author

@LeeTL1220 back to you

@LeeTL1220
Copy link
Contributor

@takutosato back to you.

@codecov-io
Copy link

codecov-io commented Sep 21, 2018

Codecov Report

Merging #5196 into master will increase coverage by 0.007%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##              master     #5196       +/-   ##
===============================================
+ Coverage     86.791%   86.798%   +0.007%     
+ Complexity     29723     29711       -12     
===============================================
  Files           1820      1820               
  Lines         137462    137406       -56     
  Branches       15171     15160       -11     
===============================================
- Hits          119304    119265       -39     
+ Misses         12649     12637       -12     
+ Partials        5509      5504        -5
Impacted Files Coverage Δ Complexity Δ
...stitute/hellbender/tools/walkers/CombineGVCFs.java 93.919% <0%> (-0.199%) 64% <0%> (-2%)
...er/tools/walkers/GenotypeGVCFsIntegrationTest.java 75.61% <0%> (-0.197%) 19% <0%> (ø)
...aplotypecaller/HaplotypeCallerIntegrationTest.java 87.919% <0%> (-0.181%) 83% <0%> (+1%)
...utils/variant/GATKVariantContextUtilsUnitTest.java 85.694% <0%> (-0.154%) 156% <0%> (-4%)
...lbender/utils/variant/GATKVariantContextUtils.java 86.026% <0%> (-0.017%) 244% <0%> (-6%)
...ls/genomicsdb/GenomicsDBImportIntegrationTest.java 89.383% <0%> (+0.332%) 73% <0%> (-1%) ⬇️
...der/tools/walkers/CombineGVCFsIntegrationTest.java 87.773% <0%> (+0.485%) 23% <0%> (-1%) ⬇️
.../hellbender/tools/genomicsdb/GenomicsDBImport.java 79.098% <0%> (+1.447%) 52% <0%> (ø) ⬇️
...tools/walkers/haplotypecaller/HaplotypeCaller.java 100% <0%> (+15.789%) 24% <0%> (+1%) ⬆️

@LeeTL1220
Copy link
Contributor

@davidbenjamin @takutosato I approved this and it needs to be in the next release. What is holding up a merge?

@jamesemery jamesemery merged commit 2447dd6 into master Oct 1, 2018
@jamesemery jamesemery deleted the ts_wdl_reword branch October 1, 2018 19:02
EdwardDixon pushed a commit to EdwardDixon/gatk that referenced this pull request Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants