Parse valid VCF 4.2 with ##INFO containing Source + Version #1248

romanzenka · 2018-12-28T16:44:11Z

Description

As per bug #517 - despite VCFv4.2 spec allowing for "Source" and "Version" fields in ##INFO, htsjdk would throw an error. This fixes the issue.

Checklist

Code compiles correctly
New tests covering changes and new functionality
All tests passing
Extended the README / documentation, if necessary
Is not backward compatible (breaks binary or source compatibility)

This allows us to validate a new feature in VCFv4.2 - optional tags in ##INFO field

codecov-io · 2018-12-28T17:34:36Z

Codecov Report

Merging #1248 into master will increase coverage by 0.029%.
The diff coverage is 73.684%.

@@               Coverage Diff               @@
##              master     #1248       +/-   ##
===============================================
+ Coverage     69.324%   69.353%   +0.029%     
- Complexity      8287      8301       +14     
===============================================
  Files            555       555               
  Lines          33075     33116       +41     
  Branches        5561      5572       +11     
===============================================
+ Hits           22929     22967       +38     
- Misses          7884      7890        +6     
+ Partials        2262      2259        -3

Impacted Files	Coverage Δ	Complexity Δ
...n/java/htsjdk/variant/vcf/VCFContigHeaderLine.java	`76.471% <100%> (ø)`	`10 <1> (ø)`	⬇️
...rc/main/java/htsjdk/variant/vcf/VCFHeaderLine.java	`74.468% <100%> (+1.135%)`	`22 <0> (+2)`	⬆️
...n/java/htsjdk/variant/vcf/VCFFilterHeaderLine.java	`100% <100%> (ø)`	`5 <1> (ø)`	⬇️
...main/java/htsjdk/variant/vcf/AbstractVCFCodec.java	`73.006% <100%> (ø)`	`90 <0> (ø)`	⬇️
...n/java/htsjdk/variant/vcf/VCFSimpleHeaderLine.java	`75.61% <33.333%> (-3.877%)`	`12 <1> (ø)`
...ain/java/htsjdk/variant/vcf/VCFInfoHeaderLine.java	`83.333% <50%> (-16.667%)`	`6 <1> (+1)`
...java/htsjdk/variant/vcf/VCFCompoundHeaderLine.java	`75.51% <73.333%> (-0.1%)`	`48 <2> (+7)`
...va/htsjdk/variant/vcf/VCFHeaderLineTranslator.java	`92.593% <78.571%> (+5.093%)`	`3 <2> (+1)`	⬆️
...sjdk/samtools/util/Md5CalculatingOutputStream.java	`84.615% <0%> (+10.256%)`	`11% <0%> (+3%)`	⬆️

romanzenka · 2018-12-28T17:46:53Z

I will work on getting coverage back in shape

lbergelson · 2019-01-02T22:12:10Z

@cmnbroad Could you take a look at this? It's one of the tickets that was slated to be closed by your giant refactor branch that never went int.

lbergelson · 2019-01-02T22:13:49Z

@romanzenka Thanks for doing this! This warning might be too late, but the code coverage is really oversensitive and tends to fail at random due to some non-determinism in the test suite. So don't worry too much about pleasing it.

romanzenka · 2019-01-02T22:25:57Z

I originally wanted to reuse parts of the original PR, but I could not figure out how it fixes this particular issue (VCF adding optional fields). So I enhanced the interface (retaining backwards compatibility) so now you can specify optional tags.

cmnbroad · 2019-01-03T13:15:50Z

Yes, the original PR refactored the entire header line class hierarchy, so it would be hard to extract just a piece of it. I'll take a look at the changes here.

pshapiro4broad

Many minor comments

src/main/java/htsjdk/variant/vcf/VCFCompoundHeaderLine.java

src/main/java/htsjdk/variant/vcf/VCFHeaderLineTranslator.java

src/test/java/htsjdk/variant/vcf/VCFHeaderLineTranslatorUnitTest.java

Cleaner call avoiding code duplication Co-Authored-By: romanzenka <zenka.roman@mayo.edu>

Use diamond operator Co-Authored-By: romanzenka <zenka.roman@mayo.edu>

1) switched optional flags from Set to List, assuming there will be typically very few of them 2) comments on VCFLineParser, deprecated version without optional flags, removed the redundant 'public' 3) clearer exception in VCF4Parser when tags are out of order versus tags are completely unexpected 4) unit test checks for error message to match expected one

src/main/java/htsjdk/variant/vcf/VCFHeaderLineTranslator.java

pshapiro4broad

Thanks for the updates the code looks 👍 to me, except for the issue with the case statement fall-through.

Will also need @lbergelson or @cmnbroad to check this off too, as I'm not a VCF format expert.

cmnbroad · 2019-01-03T19:35:49Z

@romanzenka Thanks for taking this on - we've been wanting to fix it for a while. It looks you've made changes to make "source" and "version" tolerated on read, but the values aren't retained, accessible to callers, or round-tripped on write. Would you be able to add getters so these values can be retrieved from the header lines, and update the encoder to propagate them on write, with corresponding tests ? Ideally we'd implement generalized attribute support, but thats a lot more work. For now it would be ok to handle these two attributes specifically since they're recommended in the spec, but I'd be reluctant to take a change that accepts and then silently drops them.

Will fail the tests for now

cmnbroad

Thanks for the last changes - one minor minor change requested.

@lbergelson, except for one last comment on the tests, Source and Version can be round-tripped now. I think this PR strikes as good a balance as we're going to get for that without a major overhaul such as in my other PR. The only remaining question is whether we really want to deprecate the old parseLine in VCFLineParser, and the old constructor in VCFSimpleLineHeader, or just leave them. I think they're harmless to leave, though the new replacements are preferable.

src/test/java/htsjdk/variant/vcf/VCFHeaderLineTranslatorUnitTest.java

romanzenka · 2019-01-11T14:47:28Z

I think I addressed everything - the only remaining issue was deprecation of those two constructors (that are not used anymore within the library). I removed the @deprecated for now and replaced it with @see, it can always be changed back later maybe during a larger deprecation activity when you decide to clean the library up.

cmnbroad

One hopefully final request and we can get this merged.

src/main/java/htsjdk/variant/vcf/VCFHeaderLineTranslator.java

src/test/java/htsjdk/variant/vcf/VCFHeaderLineTranslatorUnitTest.java

romanzenka · 2019-01-17T17:59:48Z

I added a freebie that you may/may not like - I am tired of the MD5CalculatingOutputStream spuriously messing up my coverage, so I made it deterministic.

lbergelson · 2019-01-17T19:37:16Z

@romanzenka 👍 to fixing that. Don't worry too much about the coverage though... it's weirdly oversensitive. We need to change the settings to be more lenient.

romanzenka · 2019-01-17T19:41:55Z

@lbergelson I do not think it is even sensitivity - there is something random about how the coverage gets compared. The latest report makes no sense to me - I have not done anything with those 21 reported files. I suspect the code coverage tool itself might be confused?
What you would want to compare is coverage as it is right now versus coverage as it would be after you merge the PR. I do not think that is happening.

lbergelson · 2019-01-17T19:45:56Z

Yeah, it choose funny comparisons sometimes and I'm not certain exactly what happens. I think it compares the coverage in the last run of master vs the coverage in the pr build, which isn't necessarily in sync although it should be close.

…n/vcf_4.2_info_test

romanzenka · 2019-01-17T19:48:44Z

@lbergelson I am simply merging master for now to see what happens... my bad, that's probably best practice before accepting a PR anyway

romanzenka · 2019-01-22T16:06:02Z

@cmnbroad Do you think this could be merged before Friday 2019-01-25? We are releasing an app and it would be nice to build it on top of an official commit instead of our own branch.
I am not hoping for an official release, although that would of course be awesome!

src/test/java/htsjdk/samtools/util/Md5CalculatingOutputStreamTest.java

cmnbroad

@lbergelson this looks good to me now (except for the old copyright date), in case you want a final review.

cmnbroad · 2019-01-22T18:33:28Z

@romanzenka Thanks. Add yes, we should be able to get this merged in the next 24 hours.

lbergelson · 2019-01-22T19:10:06Z

@cmnbroad @romanzenka 👍

Thanks for working through this one. It turned out to be more work than I expected. I don't know if we'll be able to get another release out by friday. I'll see what I can do...

romanzenka · 2019-01-22T19:16:22Z

@cmnbroad @romanzenka 👍

Thanks for working through this one. It turned out to be more work than I expected. I don't know if we'll be able to get another release out by friday. I'll see what I can do...

Don't worry about it. Release when it is appropriate to release. We'll build off a particular commit.

Oh and thanks for very thorough and high-quality review!

cmnbroad · 2019-01-22T19:50:44Z

Thanks again for working through all the issues @romanzenka.

romanzenka added 2 commits December 28, 2018 10:40

Test exposing bug in VCFv4.2 parsing

759d4e5

VCFv4.2 parsing succeeds

b66e3ca

romanzenka changed the title ~~Fix bug #517~~ Fix bug #517 - fails to parse valid VCF 4.2 with INFO containing Source + Version Dec 28, 2018

romanzenka changed the title ~~Fix bug #517 - fails to parse valid VCF 4.2 with INFO containing Source + Version~~ Parse valid VCF 4.2 with ##INFO containing Source + Version Dec 28, 2018

romanzenka added 2 commits December 28, 2018 11:12

Adding optionalTags parameter

b275b05

This allows us to validate a new feature in VCFv4.2 - optional tags in ##INFO field

We test args as before, just add optional count to the end

131211e

romanzenka added 7 commits December 28, 2018 15:53

Get rid of null, use empty set instead

fe1be14

Reuse same constructor (codecov)

7562b0d

VCFHeaderLineTranslator bugfix + more tests

8125472

It is possible to get fewer values than expected (including zero)

d8aa179

Improve coverage

3e04464

Test for < and > directly in tag

3728c70

Cover extra exception case

5766688

lbergelson assigned cmnbroad Jan 2, 2019

lbergelson requested a review from cmnbroad January 2, 2019 22:11

pshapiro4broad reviewed Jan 3, 2019

View reviewed changes

pshapiro4broad and others added 4 commits January 3, 2019 10:06

Update src/main/java/htsjdk/variant/vcf/VCFHeaderLineTranslator.java

3b91a51

Cleaner call avoiding code duplication Co-Authored-By: romanzenka <zenka.roman@mayo.edu>

Update src/main/java/htsjdk/variant/vcf/VCFCompoundHeaderLine.java

1e639fa

Use diamond operator Co-Authored-By: romanzenka <zenka.roman@mayo.edu>

Stylistic change of the switch command

1e95118

pshapiro4broad reviewed Jan 3, 2019

View reviewed changes

src/main/java/htsjdk/variant/vcf/VCFHeaderLineTranslator.java Outdated Show resolved Hide resolved

pshapiro4broad approved these changes Jan 3, 2019

View reviewed changes

Fixing a bug when parsing < and > not at the start/end

58f9810

Updated unit tests to pass extra tags as per VCF spec

fef5ea7

Will fail the tests for now

cmnbroad requested changes Jan 10, 2019

View reviewed changes

src/test/java/htsjdk/variant/vcf/VCFHeaderLineTranslatorUnitTest.java Show resolved Hide resolved

src/test/java/htsjdk/variant/vcf/VCFHeaderLineTranslatorUnitTest.java Outdated Show resolved Hide resolved

romanzenka added 2 commits January 10, 2019 09:53

Extra fields are now okay as long as they follow expected + recommended

2367420

Test roundtrip with source/version on other metadata lines

189f8d6

Turned @deprecated into @see annotations

8603ba8

cmnbroad requested changes Jan 15, 2019

View reviewed changes

src/main/java/htsjdk/variant/vcf/VCFHeaderLineTranslator.java Outdated Show resolved Hide resolved

src/test/java/htsjdk/variant/vcf/VCFHeaderLineTranslatorUnitTest.java Outdated Show resolved Hide resolved

romanzenka added 4 commits January 15, 2019 15:13

Reverting the switch statement to original state

b0b8b4a

Forgotten code to revert

1cd37f9

Reverted test cases, making sure they pass

5b0b3bc

Getting rid of spurious coverage failures on md5 output stream

bc9f999

Merge branch 'master' of https://github.com/samtools/htsjdk into roma…

03ea8f4

…n/vcf_4.2_info_test

cmnbroad reviewed Jan 22, 2019

View reviewed changes

src/test/java/htsjdk/samtools/util/Md5CalculatingOutputStreamTest.java Outdated Show resolved Hide resolved

cmnbroad approved these changes Jan 22, 2019

View reviewed changes

Bumped the copyright version year up

d59f67c

lbergelson approved these changes Jan 22, 2019

View reviewed changes

cmnbroad merged commit 93250d5 into samtools:master Jan 22, 2019

romanzenka mentioned this pull request Jan 23, 2019

Support Tabix knausb/vcfR#125

Open

cmnbroad mentioned this pull request Mar 12, 2019

BCF version detection is too permissive #1323

Closed

cmnbroad mentioned this pull request May 1, 2019

VCF4Parser.parseLine throws exception on optional Source and Version tags in INFO header #517

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse valid VCF 4.2 with ##INFO containing Source + Version #1248

Parse valid VCF 4.2 with ##INFO containing Source + Version #1248

romanzenka commented Dec 28, 2018 •

edited

Loading

codecov-io commented Dec 28, 2018 •

edited

Loading

romanzenka commented Dec 28, 2018

lbergelson commented Jan 2, 2019

lbergelson commented Jan 2, 2019

romanzenka commented Jan 2, 2019

cmnbroad commented Jan 3, 2019

pshapiro4broad left a comment

pshapiro4broad left a comment

cmnbroad commented Jan 3, 2019

cmnbroad left a comment

romanzenka commented Jan 11, 2019 •

edited

Loading

cmnbroad left a comment

romanzenka commented Jan 17, 2019

lbergelson commented Jan 17, 2019

romanzenka commented Jan 17, 2019

lbergelson commented Jan 17, 2019

romanzenka commented Jan 17, 2019 •

edited

Loading

romanzenka commented Jan 22, 2019

cmnbroad left a comment

cmnbroad commented Jan 22, 2019

lbergelson commented Jan 22, 2019

romanzenka commented Jan 22, 2019 •

edited

Loading

cmnbroad commented Jan 22, 2019

Parse valid VCF 4.2 with ##INFO containing Source + Version #1248

Parse valid VCF 4.2 with ##INFO containing Source + Version #1248

Conversation

romanzenka commented Dec 28, 2018 • edited Loading

Description

Checklist

codecov-io commented Dec 28, 2018 • edited Loading

Codecov Report

romanzenka commented Dec 28, 2018

lbergelson commented Jan 2, 2019

lbergelson commented Jan 2, 2019

romanzenka commented Jan 2, 2019

cmnbroad commented Jan 3, 2019

pshapiro4broad left a comment

Choose a reason for hiding this comment

pshapiro4broad left a comment

Choose a reason for hiding this comment

cmnbroad commented Jan 3, 2019

cmnbroad left a comment

Choose a reason for hiding this comment

romanzenka commented Jan 11, 2019 • edited Loading

cmnbroad left a comment

Choose a reason for hiding this comment

romanzenka commented Jan 17, 2019

lbergelson commented Jan 17, 2019

romanzenka commented Jan 17, 2019

lbergelson commented Jan 17, 2019

romanzenka commented Jan 17, 2019 • edited Loading

romanzenka commented Jan 22, 2019

cmnbroad left a comment

Choose a reason for hiding this comment

cmnbroad commented Jan 22, 2019

lbergelson commented Jan 22, 2019

romanzenka commented Jan 22, 2019 • edited Loading

cmnbroad commented Jan 22, 2019

romanzenka commented Dec 28, 2018 •

edited

Loading

codecov-io commented Dec 28, 2018 •

edited

Loading

romanzenka commented Jan 11, 2019 •

edited

Loading

romanzenka commented Jan 17, 2019 •

edited

Loading

romanzenka commented Jan 22, 2019 •

edited

Loading