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

Updating MarkDuplicatesSpark tiebreaking to reflect changes in picard #5011

Merged
merged 4 commits into from
Aug 13, 2018

Conversation

jamesemery
Copy link
Collaborator

@jamesemery jamesemery commented Jul 13, 2018

Tests ported from picard: broadinstitute/picard#1195

Fixes #4707

@codecov-io
Copy link

codecov-io commented Jul 13, 2018

Codecov Report

Merging #5011 into master will increase coverage by 25.921%.
The diff coverage is 95.349%.

@@               Coverage Diff                @@
##              master     #5011        +/-   ##
================================================
+ Coverage     60.157%   86.078%   +25.921%     
- Complexity     12768     29356     +16588     
================================================
  Files           1095      1783       +688     
  Lines          64608    134963     +70355     
  Branches       10395     15463      +5068     
================================================
+ Hits           38866    116173     +77307     
+ Misses         21504     13350      -8154     
- Partials        4238      5440      +1202
Impacted Files Coverage Δ Complexity Δ
...ils/read/markduplicates/sparkrecords/Fragment.java 93.75% <ø> (+6.25%) 7 <0> (+1) ⬆️
...r/utils/read/markduplicates/sparkrecords/Pair.java 100% <ø> (+7.317%) 27 <0> (+4) ⬆️
.../AbstractMarkDuplicatesCommandLineProgramTest.java 99.784% <100%> (ø) 82 <7> (?)
...forms/markduplicates/MarkDuplicatesSparkUtils.java 91.364% <100%> (+0.368%) 65 <7> (+2) ⬆️
...s/sparkrecords/TransientFieldPhysicalLocation.java 88.235% <88.235%> (ø) 9 <9> (?)
...der/tools/walkers/variantutils/SelectVariants.java 78.719% <0%> (-0.563%) 155% <0%> (+33%)
...nstitute/hellbender/engine/MultiVariantWalker.java 96.154% <0%> (-0.142%) 24% <0%> (+12%)
...r/tools/walkers/annotator/ClippingRankSumTest.java 100% <0%> (ø) 7% <0%> (+3%) ⬆️
...r/tools/walkers/bqsr/ApplyBQSRIntegrationTest.java 79.104% <0%> (ø) 17% <0%> (?)
...er/PostprocessGermlineCNVCallsIntegrationTest.java 96.842% <0%> (ø) 18% <0%> (?)
... and 1279 more

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

@jamesemery Documentation comments. Looks good otherwise. Do you have any sense if the short circuiting for single pairs saves us any time?

import picard.sam.util.PhysicalLocation;

/**
* A common interface for holding the fields in PhysicalLocation
Copy link
Member

Choose a reason for hiding this comment

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

this isn't an interface


// Methods for OpticalDuplicateFinder.PhysicalLocation
@Override
public short getReadGroup() { return this.readGroupIndex; }
Copy link
Member

Choose a reason for hiding this comment

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

it's weird that these read group isn't transient but the others are, should you javadoc that?

@lbergelson lbergelson assigned jamesemery and unassigned lbergelson Jul 18, 2018
@lbergelson lbergelson merged commit 0f5b253 into master Aug 13, 2018
@lbergelson lbergelson deleted the je_updateTiebreakingInMD branch August 13, 2018 21:07
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