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
fixed incoherent unit test cases in allele subsetting utils #6448
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More comments than I expected. There are a few test cases that still need changes. Also IntelliJ tells me some of the AD/PL variables are unused.
...ava/org/broadinstitute/hellbender/tools/walkers/genotyper/AlleleSubsettingUtilsUnitTest.java
Outdated
Show resolved
Hide resolved
...ava/org/broadinstitute/hellbender/tools/walkers/genotyper/AlleleSubsettingUtilsUnitTest.java
Show resolved
Hide resolved
...ava/org/broadinstitute/hellbender/tools/walkers/genotyper/AlleleSubsettingUtilsUnitTest.java
Outdated
Show resolved
Hide resolved
...ava/org/broadinstitute/hellbender/tools/walkers/genotyper/AlleleSubsettingUtilsUnitTest.java
Outdated
Show resolved
Hide resolved
attribute(GATKVCFConstants.STRAND_COUNT_BY_SAMPLE_KEY, new int[]{10, 10, 11, 11}).GQ(200).make())}); | ||
|
||
tests.add(new Object[]{ | ||
new VariantContextBuilder(vcBase).alleles(ACG).genotypes(new GenotypeBuilder(base).alleles(AA).AD(hetCG3AllelesAD).PL(hetCG3AllelesPL). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was wrong with this test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the ADs and PLs this case was supposed to be: triallelic (A,C,G) variant with het alt (C/G) genotype, subset to just the ref and one alt allele. But that throws an error since the genotype has both alt alleles. Previously no error was thrown because the genotype was accidentally set as hom ref, contradicting the ADs and PLs. The test as intended was inherently impossible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. You can take it out entirely then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's gone; the diff mixed-and-matched similar lines to give the appearance that part of this case remained.
Ready for round 2 of review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Axe that dead test and then this is good to go.
21c2d05
to
306dd06
Compare
306dd06
to
a4e6390
Compare
Closes #3672.
@ldgauthier The ADs, GTs, and PLs were inconsistent.