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

Making OB filtering default to true #5019

Merged
merged 5 commits into from
Jul 18, 2018
Merged

Conversation

LeeTL1220
Copy link
Contributor

@LeeTL1220 LeeTL1220 commented Jul 17, 2018

@LeeTL1220 LeeTL1220 requested a review from davidbenjamin July 17, 2018 13:10
@codecov-io
Copy link

codecov-io commented Jul 17, 2018

Codecov Report

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

@@               Coverage Diff                @@
##              master     #5019        +/-   ##
================================================
+ Coverage     60.162%   86.335%   +26.174%     
- Complexity     12772     28501     +15729     
================================================
  Files           1095      1779       +684     
  Lines          64616    132167     +67551     
  Branches       10394     14721      +4327     
================================================
+ Hits           38874    114107     +75233     
+ Misses         21504     12750      -8754     
- Partials        4238      5310      +1072
Impacted Files Coverage Δ Complexity Δ
...stitute/hellbender/utils/nio/ExtremeReadsTest.java 6.122% <0%> (ø) 2% <0%> (?)
...dateBasicSomaticShortMutationsIntegrationTest.java 100% <0%> (ø) 5% <0%> (?)
...nder/engine/filters/MetricsReadFilterUnitTest.java 100% <0%> (ø) 9% <0%> (?)
...nv/GermlineCNVIntervalVariantComposerUnitTest.java 96.61% <0%> (ø) 10% <0%> (?)
...r/tools/walkers/bqsr/ApplyBQSRIntegrationTest.java 79.104% <0%> (ø) 17% <0%> (?)
.../activityprofile/ActivityProfileStateUnitTest.java 86.667% <0%> (ø) 13% <0%> (?)
...te/hellbender/tools/PrintReadsIntegrationTest.java 96.795% <0%> (ø) 26% <0%> (?)
...s/vqsr/VariantRecalibratorModelOutputUnitTest.java 92.92% <0%> (ø) 23% <0%> (?)
...specific/AlleleSpecificAnnotationDataUnitTest.java 86.667% <0%> (ø) 4% <0%> (?)
...opynumber/CollectAllelicCountsIntegrationTest.java 100% <0%> (ø) 4% <0%> (?)
... and 1267 more

Copy link
Contributor

@davidbenjamin davidbenjamin left a comment

Choose a reason for hiding this comment

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

It looks like the default (i.e. neither run_orientation_bias_filter nor artifact_modes specified) leads to a faulty command line of -AM [nothing] in the FilterByOrientationBias task. Perhaps you could do something like Array[String] artifact_modes_to_use = if defined(artifact_modes) artifact_modes else ["C/T", "G/T"].

@LeeTL1220
Copy link
Contributor Author

I am curious why the WDL auto tests did not catch this..

@LeeTL1220
Copy link
Contributor Author

@davidbenjamin Back at you....

@davidbenjamin
Copy link
Contributor

I am curious why the WDL auto tests did not catch this..

@LeeTL1220 In scripts/m2_cromwell_tests/test_m2_wdl_multi.json we test with

"Mutect2_Multi.run_orientation_bias_filter": true`,
"Mutect2_Multi.artifact_modes": ["G/T", "C/T"],

whereas the probable bug I foresaw would only occur if these lines were omitted.

Copy link
Contributor

@davidbenjamin davidbenjamin 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 but a thought on solving #5025 in this PR.

@@ -797,6 +797,9 @@ task FilterByOrientationBias {
File pre_adapter_metrics
Array[String]? artifact_modes

# TODO: Do not pass artifact_modes as an empty array []. https://github.com/broadinstitute/gatk/issues/5025
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you solve this via (I didn't test this syntax)

Boolean run_ob_filter = select_first([run_orientation_bias_filter, true]) && length(final_artifact_modes) > 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would solve it in the workflow. Not necessarily the task if used in another workflow.

Done.

@davidbenjamin
Copy link
Contributor

Back to @LeeTL1220

@LeeTL1220
Copy link
Contributor Author

@davidbenjamin Back to you.

Copy link
Contributor

@davidbenjamin davidbenjamin 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.

@LeeTL1220 LeeTL1220 merged commit e466812 into master Jul 18, 2018
@LeeTL1220 LeeTL1220 deleted the ll_5016_default_filterOB_wdl branch July 18, 2018 20:52
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.

3 participants