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

Parse combined AS_QUALapprox values from older reblocked GVCFs properly #6442

Merged
merged 3 commits into from
Feb 10, 2020

Conversation

ldgauthier
Copy link
Contributor

Sometimes NON_REF gets a zero and sometimes it's empty. This seems isolated to a much older version of ReblockGVCF, but that was what we were running for production pipeline tests.

@droazen I'd like this to go into this week's release

@ldgauthier
Copy link
Contributor Author

@droazen this fix is the same as a bug you caught in another review -- #6079 (comment) Should be an easy review?

@ldgauthier ldgauthier requested a review from droazen February 7, 2020 15:53
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.

Two minor comments @ldgauthier, otherwise this looks good

attribute(GATKVCFConstants.AS_RAW_QUAL_APPROX_KEY, goodQualList).make();
final VariantContext withNoNonRefValue = new VariantContextBuilder(GATKVariantContextUtils.makeFromAlleles("good", "chr1", 10001, Arrays.asList("A","T", Allele.NON_REF_STRING))).
attribute(GATKVCFConstants.AS_RAW_QUAL_APPROX_KEY, trickyQualList).make();
Assert.assertTrue(AS_QualByDepth.parseQualList(withNonRefValue).size() == withNonRefValue.getAlternateAlleles().size());
Copy link
Contributor

Choose a reason for hiding this comment

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

assertEquals() would be more appropriate here

attribute(GATKVCFConstants.AS_RAW_QUAL_APPROX_KEY, trickyQualList).make();
Assert.assertTrue(AS_QualByDepth.parseQualList(withNonRefValue).size() == withNonRefValue.getAlternateAlleles().size());
Assert.assertTrue(AS_QualByDepth.parseQualList(withNoNonRefValue).size() == withNonRefValue.getAlternateAlleles().size());
}
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 also add an assertion that the actual QD value assigned to NON_REF was 0 in the withNoNonRefValue case?

@ldgauthier ldgauthier assigned droazen and unassigned ldgauthier Feb 7, 2020
@ldgauthier
Copy link
Contributor Author

@droazen back to you

@gatk-bot
Copy link

gatk-bot commented Feb 7, 2020

Travis reported job failures from build 29045
Failures in the following jobs:

Test Type JDK Job ID Logs
unit openjdk11 29045.13 logs
unit openjdk8 29045.3 logs

@droazen
Copy link
Contributor

droazen commented Feb 7, 2020

@ldgauthier Looks like you have a test failure in AS_QualByDepthUnitTest.testParseQualList (https://storage.googleapis.com/hellbender-test-logs/build_reports/master_29045.13/tests/test/index.html)

@ldgauthier
Copy link
Contributor Author

@droazen I immediately pushed another commit -- see "Derp" above. Looks green now.

@droazen droazen merged commit 9efe23d into master Feb 10, 2020
@droazen droazen deleted the ldg_GnarlyParsingBug branch February 10, 2020 18:29
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