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

Fix ReblockGVCF NPE on no-calls #5863

Closed
wants to merge 9 commits into from
Closed

Conversation

ldgauthier
Copy link
Contributor

No description provided.

@ldgauthier ldgauthier force-pushed the ldg_reblockNoCalls branch from 54f4aa5 to 3988246 Compare April 5, 2019 19:43
@codecov-io
Copy link

codecov-io commented Apr 5, 2019

Codecov Report

Merging #5863 into master will decrease coverage by 73.464%.
The diff coverage is 4.695%.

@@               Coverage Diff                @@
##              master     #5863        +/-   ##
================================================
- Coverage     86.841%   13.378%   -73.464%     
+ Complexity     32327      6386     -25941     
================================================
  Files           1991      2011        +20     
  Lines         149347    151036      +1689     
  Branches       16482     16151       -331     
================================================
- Hits          129695     20205    -109490     
- Misses         13646    128567    +114921     
+ Partials        6006      2264      -3742
Impacted Files Coverage Δ Complexity Δ
...lkers/annotator/allelespecific/AS_QualByDepth.java 1.887% <ø> (-73.585%) 1 <0> (-16)
...ute/hellbender/utils/variant/GATKVCFConstants.java 50% <ø> (-25%) 2 <0> (-2)
...er/engine/spark/datasources/VariantsSparkSink.java 32.353% <ø> (-45.772%) 3 <0> (-5)
...te/hellbender/utils/variant/writers/TLODBlock.java 0% <ø> (-66.667%) 0 <0> (-8)
...ender/utils/variant/writers/GVCFBlockCombiner.java 0% <0%> (-93.333%) 0 <0> (-35)
...ender/utils/variant/writers/TLODBlockUnitTest.java 4.348% <0%> (-89.13%) 2 <0> (-7)
...ls/variant/writers/GVCFBlockCombiningIterator.java 0% <0%> (-100%) 0 <0> (-1)
...ools/walkers/variantutils/ReblockGVCFUnitTest.java 6.579% <0%> (-85.958%) 2 <0> (-7)
...tils/variant/writers/SomaticGVCFBlockCombiner.java 0% <0%> (-90.476%) 0 <0> (-12)
...e/hellbender/utils/variant/writers/GVCFWriter.java 0% <0%> (-100%) 0 <0> (-8)
... and 1821 more

Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

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

Back to @ldgauthier with some comments

return Collections.emptyMap();
}
final Pair<Integer, Double> sampleCountCoeff = calculateIC(vc, genotypes);
final int sampleCount = sampleCountCoeff.getLeft();
final double F = sampleCountCoeff.getRight();
if (sampleCount < MIN_SAMPLES) {
logger.warn("Annotation will not be calculated, must provide at least " + MIN_SAMPLES + " samples");
logger.warn("InbreedingCoeff will not be calculated for at least one position; at least " + MIN_SAMPLES + " samples must have called genotypes");
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you want separate "one shot" loggers for these 2 warnings? (ie., would you ever want both warnings to be emitted?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first one is like "hey, you don't have enough samples in your cohort" and the second one is like "oops, there are a lot of no-calls at this site, so I'm not going to output a value here", so they should be mutually exclusive.

final Genotype g2 = makeG("sample1", Allele.NO_CALL,Allele.NO_CALL);
final VariantContext noData = makeDeletionVC("noData", Arrays.asList(LONG_REF, DELETION, Allele.NON_REF_ALLELE), LONG_REF.length(), g2);
final VariantContext notCrashing = reblocker.lowQualVariantToGQ0HomRef(noData, noData);
Assert.assertTrue(notCrashing.getGenotype(0).isNoCall());
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment explaining this test case (ie., explain what the "crash" being tested for was)

final Genotype g2 = makeG("sample1", Allele.NO_CALL,Allele.NO_CALL);
final VariantContext noData = makeDeletionVC("noData", Arrays.asList(LONG_REF, DELETION, Allele.NON_REF_ALLELE), LONG_REF.length(), g2);
final VariantContext notCrashing = reblocker.lowQualVariantToGQ0HomRef(noData, noData);
Assert.assertTrue(notCrashing.getGenotype(0).isNoCall());
}

@Test
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 add a test case covering the change to shouldBeReblocked() as well (and make that method @VisibleForTesting?)

Copy AS annotations too
Put RAW_MQandDP back to int tuple so GDB doesn't duck it up
Fix MQ bug, upgrade from experimental to beta
@ldgauthier ldgauthier requested a review from droazen April 24, 2019 20:30
@ldgauthier
Copy link
Contributor Author

@droazen I addressed your comments, but I also added some more fixes (and tests!)

@droazen droazen assigned droazen and unassigned ldgauthier Apr 24, 2019
@droazen
Copy link
Contributor

droazen commented Aug 1, 2019

Closing this one -- these changes to ReblockGVCFs went in as part of #4947

@droazen droazen closed this Aug 1, 2019
@droazen droazen deleted the ldg_reblockNoCalls branch August 1, 2019 16:06
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