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

ValidateVariants exception message improvement #6076

Merged
merged 4 commits into from
Oct 30, 2019

Conversation

ldgauthier
Copy link
Contributor

I added a test according to #4642 , but can't reproduce the error. The user also noted that the error message was badly formed, which is true because it ended with a colon. Now it looks like:
"Input src/test/resources/org/broadinstitute/hellbender/tools/walkers/ValidateVariants/validationExampleBad.vcf fails strict validation of type CHR_COUNTS: the Allele Count (AC) tag is incorrect for the record at position 1:985447, 1 vs. 2"

@samuelklee
Copy link
Contributor

samuelklee commented Aug 2, 2019

Thanks for fixing the message! I couldn't track down the original output.vcf that caused the issue, and this is ancient enough that I can't even recall the context. However, because the site 1262288 only appears in our repo in the simulated gCNV test data, I suspect that this VCF was generated at some point during development of VCF output for PostprocessGermlineCNVCalls. Not sure in what way that VCF might have been invalid (I'm pretty sure the VCFs produced by that tool now are valid), but I can try to reproduce when I get a chance and will reopen if necessary.

@ldgauthier ldgauthier force-pushed the ldg_validateVariantsBug branch from 3321d15 to 84b9e10 Compare August 5, 2019 18:46
@droazen droazen requested a review from samuelklee August 5, 2019 20:16
Copy link
Contributor

@samuelklee samuelklee left a comment

Choose a reason for hiding this comment

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

Thanks! Just one very minor comment, otherwise looks good to me. Also, I wonder if the issue that prompted this was fixed in #5984; see discussion in #5862.

@@ -160,6 +160,15 @@ public void testBadChrCount1() throws IOException {
);

spec.executeTest("test bad chr counts #1", this);

//test with no reference and no validations specified
IntegrationTestSpec spec2 = new IntegrationTestSpec(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could go through all of the tests and make these final, but I'll leave it up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adam put these tests in and he was missing finals???
I did the two in the test I modified, but I don't like having my git blame all over the place if I didn't substantively modify the tests.

@ldgauthier
Copy link
Contributor Author

@samuelklee I bet you were right about Jonathan's latest fix, but the error message did still need a little polish.
Good to go?

Copy link
Contributor

@samuelklee samuelklee left a comment

Choose a reason for hiding this comment

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

Whoops, guess I never approved this? Apologies, @ldgauthier!

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.

2 participants