Skip to content

Correct irrelevant inheritance in Mutect2 #5758

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

Merged
merged 5 commits into from
Mar 8, 2019
Merged

Conversation

davidbenjamin
Copy link
Contributor

Closes #5352. This removes unused command line arguments from Mutect2.

@LeeTL1220 This PR looks big but it's really just a lot of moving around static methods and giving HaplotypeCallerArgumentCollection a StandardCallerCollection as a member instead of havingAssemblyBasedCallerCollection inherit from it.

@davidbenjamin davidbenjamin added this to the Mutect 3 milestone Mar 5, 2019
@davidbenjamin davidbenjamin requested a review from LeeTL1220 March 5, 2019 19:56
@@ -133,7 +103,7 @@ public ReadThreadingAssembler createReadThreadingAssembler() {

@Advanced
@Argument(fullName = SMITH_WATERMAN_LONG_NAME, doc = "Which Smith-Waterman implementation to use, generally FASTEST_AVAILABLE is the right choice", optional = true)
public SmithWatermanAligner.Implementation smithWatermanImplementation = SmithWatermanAligner.Implementation.JAVA;
public SmithWatermanAligner.Implementation smithWatermanImplementation = SmithWatermanAligner.Implementation.FASTEST_AVAILABLE;
Copy link
Contributor

Choose a reason for hiding this comment

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

@davidbenjamin Did you intend for this PR to make substantive changes to default values for arguments? You present it as a pure refactoring branch, but it looks like there are changes like this embedded in it. We deliberately have not yet turned the native SmithWaterman on by default due to our current inability to continuously test it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@droazen That's an accident -- I was just curious about its effect on the M2 integration test runtime and forgot to revert. I intend everything to be refactoring. This does include simplifying the assembly argument collections, which is not strictly necessary for the purposes of this PR. I could strip out things like that if you would prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, thanks for the clarification! Can you do a pass to check that additional changes like this didn't slip in (ie., changes to argument defaults)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely!

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 was the only one, fortunately.

@droazen droazen self-assigned this Mar 5, 2019
@codecov-io
Copy link

codecov-io commented Mar 5, 2019

Codecov Report

Merging #5758 into master will increase coverage by 0.015%.
The diff coverage is 93.526%.

@@               Coverage Diff               @@
##              master     #5758       +/-   ##
===============================================
+ Coverage     86.983%   86.998%   +0.015%     
  Complexity     31863     31863               
===============================================
  Files           1943      1942        -1     
  Lines         146775    146750       -25     
  Branches       16225     16215       -10     
===============================================
  Hits          127669    127669               
+ Misses         13192     13168       -24     
+ Partials        5914      5913        -1
Impacted Files Coverage Δ Complexity Δ
...itute/hellbender/tools/walkers/mutect/Mutect2.java 84.091% <ø> (ø) 22 <0> (ø) ⬇️
...r/tools/walkers/mutect/Mutect2IntegrationTest.java 91.993% <ø> (ø) 92 <0> (ø) ⬇️
...stitute/hellbender/tools/HaplotypeCallerSpark.java 70.115% <ø> (ø) 18 <0> (ø) ⬇️
...aller/HaplotypeCallerGenotypingEngineUnitTest.java 80.693% <ø> (-8.808%) 18 <0> (-12)
.../tools/walkers/mutect/SomaticGenotypingEngine.java 92.462% <100%> (+2.413%) 75 <3> (-1) ⬇️
...otypecaller/HaplotypeCallerArgumentCollection.java 100% <100%> (ø) 3 <0> (ø) ⬇️
...ller/ReadThreadingAssemblerArgumentCollection.java 96% <100%> (+1.556%) 1 <0> (ø) ⬇️
...ecaller/AssemblyBasedCallerArgumentCollection.java 100% <100%> (ø) 2 <1> (ø) ⬇️
...der/tools/walkers/mutect/M2ArgumentCollection.java 89.744% <100%> (+0.27%) 14 <0> (ø) ⬇️
...bender/tools/walkers/variantutils/ReblockGVCF.java 81.522% <100%> (ø) 46 <0> (ø) ⬇️
... and 13 more

Copy link
Contributor

@LeeTL1220 LeeTL1220 left a comment

Choose a reason for hiding this comment

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

Given our offline conversation/walkthrough, this seems reasonable.

You may want to write a note about the convention of specifying alleles to get GGA mode in docs.

In the docs for the Mutect2 command.

After that, feel free to merge.

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