Tolerate lower-case nans in QUAL #1364

karenfeng · 2019-05-08T19:00:26Z

Description

We sometimes see VCFs with lower-case nan's in QUAL. This doesn't match the Java-style NaN, and therefore causes VCF parsing to break.

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)

codecov-io · 2019-05-08T19:15:10Z

Codecov Report

Merging #1364 into master will increase coverage by 0.036%.
The diff coverage is 89.474%.

@@               Coverage Diff               @@
##              master     #1364       +/-   ##
===============================================
+ Coverage     68.007%   68.043%   +0.036%     
- Complexity      8352      8368       +16     
===============================================
  Files            570       571        +1     
  Lines          33848     33889       +41     
  Branches        5661      5665        +4     
===============================================
+ Hits           23019     23059       +40     
+ Misses          8645      8640        -5     
- Partials        2184      2190        +6

Impacted Files	Coverage Δ	Complexity Δ
...n/java/htsjdk/variant/variantcontext/Genotype.java	`59.036% <0%> (ø)`	`80 <0> (ø)`	⬇️
.../htsjdk/variant/variantcontext/VariantContext.java	`77.714% <100%> (ø)`	`246 <0> (ø)`	⬇️
...dk/variant/variantcontext/GenotypeLikelihoods.java	`84.971% <100%> (ø)`	`74 <0> (ø)`	⬇️
...main/java/htsjdk/variant/vcf/AbstractVCFCodec.java	`76.218% <100%> (+0.573%)`	`100 <0> (+2)`	⬆️
src/main/java/htsjdk/variant/vcf/VCFUtils.java	`49.275% <100%> (+4.831%)`	`22 <5> (+5)`	⬆️
...java/htsjdk/variant/variantcontext/CommonInfo.java	`47.863% <50%> (ø)`	`47 <0> (ø)`	⬇️
...htsjdk/samtools/util/nio/DeleteOnExitPathHook.java	`78.947% <0%> (-10.526%)`	`3% <0%> (-1%)`
...dk/samtools/cram/structure/SubstitutionMatrix.java	`88.189% <0%> (-3.564%)`	`25% <0%> (+10%)`
...samtools/cram/structure/CramCompressionRecord.java	`92.424% <0%> (-0.545%)`	`114% <0%> (+1%)`
.../samtools/cram/build/CompressionHeaderFactory.java	`89.6% <0%> (-0.134%)`	`59% <0%> (-5%)`
... and 4 more

yfarjoun · 2019-05-08T19:33:08Z

does the spec allow for Nan in the QUAL field?

…

On Wed, May 8, 2019 at 3:00 PM Karen Feng ***@***.***> wrote: Description We sometimes see VCFs with lower-case nan's in QUAL. This doesn't match the Java-style NaN, and therefore causes VCF parsing to break. 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) ------------------------------ You can view, comment on, or merge this pull request online at: #1364 Commit Summary - Tolerate lower-case nans File Changes - *M* src/main/java/htsjdk/variant/vcf/AbstractVCFCodec.java <https://github.com/samtools/htsjdk/pull/1364/files#diff-0> (16) - *M* src/test/java/htsjdk/variant/vcf/AbstractVCFCodecTest.java <https://github.com/samtools/htsjdk/pull/1364/files#diff-1> (15) - *A* src/test/resources/htsjdk/variant/test_withNanQual.vcf <https://github.com/samtools/htsjdk/pull/1364/files#diff-2> (8) Patch Links: - https://github.com/samtools/htsjdk/pull/1364.patch - https://github.com/samtools/htsjdk/pull/1364.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1364>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAU6JUU4IAD2YZS4UI7HP7DPUMPMZANCNFSM4HLULL3A> .

karenfeng · 2019-05-08T20:39:07Z

VCF reading doesn't break on NaN in the QUAL or GQ fields (as in the associated unit test), but I'm not completely confident on how spec-compliant that is. The spec seems to suggest that because QUAL is a Float, it should be ok; the reason GQ doesn't break on NaN today is because it's parsed via Double rather than Integer.

jmarshall · 2019-05-08T21:30:47Z

The 4.3 spec is clear that NaNs are valid in QUAL (§1.6.1 states QUAL is a Float; §1.3 says VCF Floats include NaN). The earlier specs don't have §1.3 and appear to be silent on what text a Float might allow.

See also NaN vs. nan discussions in samtools/bcftools#755 (comment) and samtools/hts-specs#89 (comment) and nearby.

yfarjoun · 2019-05-13T21:25:47Z

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

+     * Parses a String as a Double, being tolerant for lower-case NaN (nan).
+     */
+    private static final Double decodeDouble(final String string) {
+        if (Pattern.matches("[+-]?nan", string)) {


I don't see -+ nans defined anywhere, only "nan" without a sign...have you seen that in the wild? -+Infinity I have seen...but not NaN.

Could you please:

first attempt to call Double.valueOf(string) and catch the exception if it fails (for performance)

compile the pattern into a static final (also for performance)

make the pattern match case-independent Pattern.compile(regex, Pattern.CASE_INSENSITIVE) and remove the [+-] part.

also add checks (and tests) for [-+]?inf(inity)? and encode into either Double.POSITIVE_INFINITY or Double.NEGATIVE_INFINITY

Thanks!

NaNs are NaNs and their sign is surely meaningless, but nonetheless the IEEE NaN format has a sign bit and C printf will print it (in glibc and other implementations, but not the BSD/macOS one — though IMHO that's not standard compliant). And Java will parse it.

So IMHO you should keep the [+-]? part. My suggestion for what the VCF spec should allow for NaN/∞ is samtools/hts-specs#409.

I'd like to object to allowing ∞...

Some additional comments:

Replying to @yfarjoun point 1: Usually I'm not a fan of catching exceptions as part of the standard flow of a method, but in this case I agree that it makes sense for performance reasons.

This should be a replacement for Double.parseDouble and return a primitive double instead of the boxed Double

After digging out the code that implements Double.parseDouble (called by valueOf anyways) it turns out to be such monstrosity start from parseDouble in here that a quick checks to identify "nan" (or "infinity") with charAt(length()-1) == 'n' might be better than surrounding with a try-catch ... I don't know what the performance penalty depending on what penalty you paying for wrapping a mostly successful code in a try { ... }.

lbergelson

@karenfeng Thank you for this PR. It's a good idea. We've made some suggestions for changes before this can be merged.

lbergelson · 2019-05-15T18:35:50Z

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

+    /**
+     * Parses a String as a Double, being tolerant for lower-case NaN (nan).
+     */
+    private static final Double decodeDouble(final String string) {


I would expose this somewhere public and maybe rename it to be clearer how it's different from the standard methods. Maybe VCFUtils.parseDoubleAccordingToVcfSpec.

There are a number of additional places that this should be used as well I believe:

GenotypeLikelihoods.parseDeprecatedGLString line 270 parseDouble CommonInfo299 and 323 that useDouble.valueOf VariantContext1632Genotype` 529

karenfeng · 2019-05-22T22:27:51Z

@lbergelson Thank you for the feedback; let me know if there any further changes you would like me to make.

yfarjoun

Thanks for the changes, I think that the point was missed regarding how parseDouble should be used to avoid boxing...

src/main/java/htsjdk/variant/variantcontext/Genotype.java

src/main/java/htsjdk/variant/variantcontext/VariantContext.java

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

src/test/resources/htsjdk/variant/test_withNanQual.vcf

…parse-double-nan

pshapiro4broad · 2019-06-20T18:34:02Z

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

 import java.util.stream.Collectors;

 public class VCFUtils {

+    private static Pattern infOrNanPattern = Pattern.compile("^(?<sign>[-+]?)((?<inf>(INF|INFINITY))|(?<nan>NAN))$", Pattern.CASE_INSENSITIVE);


this should be final, and should have an uppercase name since it's a static, e.g.

Suggested change

private static Pattern infOrNanPattern = Pattern.compile("^(?<sign>[-+]?)((?<inf>(INF|INFINITY))|(?<nan>NAN))$", Pattern.CASE_INSENSITIVE);

private static final Pattern INF_OR_NAN_PATTERN = Pattern.compile("^(?<sign>[-+]?)((?<inf>(INF|INFINITY))|(?<nan>NAN))$", Pattern.CASE_INSENSITIVE);

pshapiro4broad · 2019-06-20T18:35:28Z

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

+    /**
+     * Parses a String as a Double, being tolerant for lower-case NaN (nan).
+     */
+    public static double parseDoubleAccordingToVcfSpec(final String str) {


I realize I'm late to this PR, but how about parseVcfDouble()? Saying "according to spec" seems redundant to me, unless we need to distinguish this from a VCF double parsing that's not done according to a spec.

pshapiro4broad · 2019-06-20T18:36:47Z

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

@@ -250,6 +254,32 @@ else if (vcfFile.getAbsolutePath().endsWith(IOUtil.COMPRESSED_VCF_FILE_EXTENSION
        return output;
    }

+    /**
+     * Parses a String as a Double, being tolerant for lower-case NaN (nan).


This javadoc doesn't seem sufficient given that this supports INF and NaN. It would be nice to document its behavior more completely, and/or provide a link to part of the spec that this code is implementing.

pshapiro4broad · 2019-06-20T18:38:26Z

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

+                    }
+                }
+                return ret;
+            } else {


unnecessary else after return

pshapiro4broad · 2019-06-20T18:40:15Z

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

+                    ret = Double.NaN;
+                } else {
+                    if (matcher.group("sign").equals("-")) {
+                        ret = Double.NEGATIVE_INFINITY;


The test below appears to test this case but it's showing up as not covered. Are you sure that this code path is being tested by the code below?

pshapiro4broad · 2019-06-20T18:41:23Z

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

+    }
+
+    @Test(dataProvider = "caseIntolerantDoubles")
+    public void testCaseIntolerantDoubles(String vcfInput, Double value) {


unnecessary to use a boxed type here as value is never null

Suggested change

public void testCaseIntolerantDoubles(String vcfInput, Double value) {

public void testCaseIntolerantDoubles(String vcfInput, double value) {

pshapiro4broad · 2019-06-20T18:42:42Z

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

+                }
+                return ret;
+            } else {
+                throw e;


Can you add a unit test to VCFUtilsTest that verifies this negative case?

lbergelson

This looks good to me now. Thank you @karenfeng

Yossi gave verbal thumbs up.

alecw · 2019-07-10T19:44:57Z

Hi @lbergelson , is a release with this change coming any time soon? We would really appreciate it.

Thanks, Alec

lbergelson · 2019-07-10T20:05:00Z

Yes. I've wanted to do a release for a while but I've been busy with personal stuff (mostly baby related) and because of incompatibilities I've had trouble running the downstream tests successfully so it's taken longer than intended. I hope to have one out this week but next week at the latest. I can publish a release candidate that hasn't been fully tested if that would be helpful.

Tolerate lower-case nans

62519d3

yfarjoun requested a review from lbergelson May 10, 2019 14:35

yfarjoun reviewed May 13, 2019

View reviewed changes

lbergelson requested changes May 15, 2019

View reviewed changes

lbergelson assigned karenfeng May 15, 2019

lbergelson added the Waiting for revisions This PR has received comments from reviewers and is waiting for the Author to respond label May 15, 2019

Address comments

4cd00ad

yfarjoun previously requested changes Jun 3, 2019

View reviewed changes

yfarjoun mentioned this pull request Jun 17, 2019

Handle "nan" in Float attribute regardless of case. #1383

Closed

5 tasks

karenfeng added 3 commits June 18, 2019 11:21

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

af6ce97

…parse-double-nan

Include Inf, Infinity

9392b6b

One more

d51cec4

pshapiro4broad reviewed Jun 20, 2019

View reviewed changes

Add more tests

c54e40b

pshapiro4broad approved these changes Jun 24, 2019

View reviewed changes

lbergelson approved these changes Jun 24, 2019

View reviewed changes

lbergelson merged commit 37ea940 into samtools:master Jun 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tolerate lower-case nans in QUAL #1364

Tolerate lower-case nans in QUAL #1364

karenfeng commented May 8, 2019 •

edited

Loading

codecov-io commented May 8, 2019 •

edited

Loading

yfarjoun commented May 8, 2019 via email

karenfeng commented May 8, 2019 •

edited

Loading

jmarshall commented May 8, 2019

yfarjoun May 13, 2019

jmarshall May 13, 2019

lbergelson May 15, 2019

lbergelson May 15, 2019

vruano May 19, 2019

lbergelson left a comment

lbergelson May 15, 2019

lbergelson May 15, 2019

karenfeng commented May 22, 2019

yfarjoun left a comment

pshapiro4broad Jun 20, 2019

pshapiro4broad Jun 20, 2019

pshapiro4broad Jun 20, 2019

pshapiro4broad Jun 20, 2019

pshapiro4broad Jun 20, 2019

pshapiro4broad Jun 20, 2019

pshapiro4broad Jun 20, 2019

lbergelson left a comment

alecw commented Jul 10, 2019

lbergelson commented Jul 10, 2019

	private static Pattern infOrNanPattern = Pattern.compile("^(?<sign>[-+]?)((?<inf>(INF\|INFINITY))\|(?<nan>NAN))$", Pattern.CASE_INSENSITIVE);
	private static final Pattern INF_OR_NAN_PATTERN = Pattern.compile("^(?<sign>[-+]?)((?<inf>(INF\|INFINITY))\|(?<nan>NAN))$", Pattern.CASE_INSENSITIVE);

	public void testCaseIntolerantDoubles(String vcfInput, Double value) {
	public void testCaseIntolerantDoubles(String vcfInput, double value) {

Tolerate lower-case nans in QUAL #1364

Tolerate lower-case nans in QUAL #1364

Conversation

karenfeng commented May 8, 2019 • edited Loading

Description

Checklist

codecov-io commented May 8, 2019 • edited Loading

Codecov Report

yfarjoun commented May 8, 2019 via email

karenfeng commented May 8, 2019 • edited Loading

jmarshall commented May 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenfeng commented May 22, 2019

yfarjoun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

alecw commented Jul 10, 2019

lbergelson commented Jul 10, 2019

karenfeng commented May 8, 2019 •

edited

Loading

codecov-io commented May 8, 2019 •

edited

Loading

karenfeng commented May 8, 2019 •

edited

Loading