unsigned int is backed by a long value #339

vadimzalunin · 2015-09-30T16:12:46Z

replaced exception when reading unsigned int in ReadTag with a direct conversion to long

droazen · 2015-10-22T22:52:27Z

@vadimzalunin I've made some changes in master to make it easier to see which tests are failing in travis, and have taken the liberty of rebasing this branch on top of those changes. It's clear from doing this that the test failure in this branch is closely related to the changes you've made in it, as you should now be able to see in travis:

[testng] FAILED: test("auxf#values")
[testng] htsjdk.samtools.util.RuntimeEOFException: htsjdk.samtools.SAMException: Attribute type class java.lang.Long not supported. Tag: I9
[testng]    at htsjdk.samtools.CRAMIterator.hasNext(CRAMIterator.java:257)
[testng]    at htsjdk.samtools.CRAMComplianceTest.test(CRAMComplianceTest.java:123)
[testng] Caused by: htsjdk.samtools.SAMException: Attribute type class java.lang.Long not supported. Tag: I9
[testng]    at htsjdk.samtools.SAMRecord.setAttribute(SAMRecord.java:1210)
[testng]    at htsjdk.samtools.SAMRecord.setAttribute(SAMRecord.java:1201)
[testng]    at htsjdk.samtools.SAMRecord.setAttribute(SAMRecord.java:1177)
[testng]    at htsjdk.samtools.cram.build.Cram2SamRecordFactory.create(Cram2SamRecordFactory.java:93)
[testng]    at htsjdk.samtools.CRAMIterator.nextContainer(CRAMIterator.java:191)
[testng]    at htsjdk.samtools.CRAMIterator.hasNext(CRAMIterator.java:255)
[testng]    ... 22 more
[testng] ... Removed 21 stack frames
[testng] 
[testng] ===============================================
[testng]     Ant test
[testng]     Tests run: 22443, Failures: 1, Skips: 0
[testng] ===============================================

droazen · 2015-10-22T22:53:14Z

Back to @vadimzalunin for test fixes

mp15 · 2015-10-23T10:42:07Z

Looks like this only failing on the incorrect test testUnsignedIntegerSAM which is expecting an exception to be thrown (it shouldn't be with this fix in place). Maybe update the test?

vadimzalunin · 2015-10-23T12:07:55Z

@mp15 done

droazen · 2015-10-23T14:22:54Z

@vadimzalunin Could you please squash this branch into a single commit?

vadimzalunin · 2015-10-23T16:07:00Z

@droazen squashed

droazen · 2015-10-23T17:42:22Z

src/java/htsjdk/samtools/SAMRecord.java

@@ -1203,7 +1203,7 @@ protected void setAttribute(final short tag, final Object value) {

    protected void setAttribute(final short tag, final Object value, final boolean isUnsignedArray) {
        if (value != null &&
-                !(value instanceof Byte || value instanceof Short || value instanceof Integer ||
+                !(value instanceof Byte || value instanceof Short || value instanceof Integer || value instanceof Long ||


With this change to allow Long values directly in SAMRecord attributes, what protection do we have against a client setting an attribute outside the range of uint32_t that bams can handle? The htsjdk BAM codec will fail given a Long value that exceeds BinaryCodec.MAX_UINT.

Also, if you set such an attribute using setAttribute() and then call getIntegerAttribute() you'll get a RuntimeException if the value exceeds Integer.MAX_VALUE (even though the attribute may be marked as of type i/I, like the ia:i:4294967295 attribute in http://gatkforums.broadinstitute.org/discussion/comment/24038/#Comment_24038).

Can you comment on these concerns? I don't want to block this important fix, but I want to be sure we're not doing something here that will cause problems later on.

droazen · 2015-10-23T17:45:03Z

Back to @vadimzalunin for comments.

vadimzalunin · 2015-10-28T16:25:04Z

@droazen re-implemented the fix, basically made uint32 policy the same in CRAM as in BAM. Added dedicated test class and a BAM file.

vadimzalunin · 2015-10-29T10:30:09Z

added tests for values over max unsigned int

vadimzalunin · 2015-10-29T14:15:55Z

@droazen please review

mp15 · 2015-11-02T10:33:42Z

@vadimzalunin It won't stop an effective review but it looks like you've accidentally left the squashed branch as two commits with same message rather than one? (This has been fixed by 19e8e3b)

nh13 · 2015-11-04T20:12:15Z

src/java/htsjdk/samtools/CRAMFileWriter.java

@@ -379,6 +379,8 @@ protected void finish() {
            outputStream.flush();
            if (indexer != null)
                indexer.finish();
+        }catch (final RuntimeException re) {


nh13 · 2015-11-04T20:18:34Z

@droazen thanks for the context as it wasn't clear here, it also is timely as htsjdk states that java 7 will be supported only until October 2015, so we should feel free to move to java 8 if we need specific features there.

There seem to be a lot of Cram changes here, are those also necessary given @droazen suggestions, which seem reasonable?

droazen · 2015-11-06T20:08:57Z

@vadimzalunin Ping me once you've made the requested changes and this is ready for final review.

vadimzalunin · 2015-11-11T13:38:48Z

@droazen please review

ronlevine · 2015-11-11T15:15:34Z

src/java/htsjdk/samtools/SAMRecord.java

@@ -1159,6 +1161,32 @@ public Object getAttribute(final short tag) {
    }

    /**
+     * A convinience method that will return a valid unsigned integer or fail with an exception if the tag value is invalid.


convenience -> convenience

vadimzalunin · 2015-11-11T19:51:34Z

Sorry guys, I appreciate this has turned into a large patch. I tried to cover critical points with tests, in particular see src/tests/java/htsjdk/samtools/SAMIntegerTagTest.java
One issue is that SAM is different from BAM and CRAM.
Another issue is ambiguity in type detection, for example -1 may be a valid signed or invalid unsigned.

vadimzalunin · 2015-11-12T17:20:37Z

@droazen ping

droazen · 2015-11-12T17:50:33Z

@vadimzalunin Am going to take a look now -- while you're waiting, perhaps you could take a look at this serious-sounding CRAM bug we discovered recently?

#364

It sounds like the kind of thing that might be a quick fix (ie., a simple off-by-one error)

droazen · 2015-11-12T18:16:28Z

Looks like there are a few additional changes I'd like to see here -- since we're pressed for time, I'll make these changes myself and push them directly into this branch to spare us another code review cycle. While I do this if you could look at #364 it would be much appreciated, as we can't really recommend that anyone use GATK 3.5 with cram for anything other than testing purposes if we have bugs like that open.

droazen · 2015-11-12T23:10:58Z

Ok, I've finished making changes to this branch -- a summary of the notable changes:

-BinaryTagCodec and ReadTag return an int for I-typed attributes when possible, only returning a long for values > Integer.MAX_VALUE and <= BinaryCodec.MAX_UINT. This is slightly more conservative and less likely to break client code.

-Removed some unrelated cram changes in CRAMIterator (allowing null ReferenceSource) that conflicted with changes in #368

-Added an overload of getUnsignedIntegerAttribute() that takes a String (with tests).

-Moved isValidUnsignedInteger() to SAMUtils

-Improved error messages, documentation, and naming of test cases.

Will rebase and merge now, once tests pass.

vadimzalunin · 2015-11-12T23:12:43Z

@droazen thanks! looks good to me 👍

unsigned int is backed by a long value

coveralls · 2017-09-14T12:08:20Z

Changes Unknown when pulling ca7e1c9 on CRAM_uint32_t_in_ReadTag into ** on master**.

While it was a Google Docs document, the specification was corrected to reflect that HTTP Range byte ranges are zero-based fully-inclusive. Update the OpenAPI description correspondingly. Also fix typos.

vadimzalunin mentioned this pull request Oct 2, 2015

Unexpected failures in travis: not a regular file: /dev/null #341

Closed

droazen self-assigned this Oct 9, 2015

vadimzalunin mentioned this pull request Oct 21, 2015

uint32_t is a valid BAM type but blows up CRAM #322

Closed

droazen force-pushed the CRAM_uint32_t_in_ReadTag branch 2 times, most recently from 4ee2787 to 90bdc58 Compare October 22, 2015 22:45

droazen assigned vadimzalunin and unassigned droazen Oct 22, 2015

droazen mentioned this pull request Oct 22, 2015

improving CRAM compression #355

Closed

vadimzalunin force-pushed the CRAM_uint32_t_in_ReadTag branch from c294bf7 to 0b259ff Compare October 23, 2015 15:52

droazen assigned droazen and unassigned vadimzalunin Oct 23, 2015

droazen reviewed Oct 23, 2015
View reviewed changes

droazen assigned vadimzalunin and unassigned droazen Oct 23, 2015

vadimzalunin force-pushed the CRAM_uint32_t_in_ReadTag branch 2 times, most recently from 794f45e to 0c3f09b Compare October 28, 2015 16:02

vadimzalunin force-pushed the CRAM_uint32_t_in_ReadTag branch from 0c3f09b to ed25614 Compare October 29, 2015 10:29

vadimzalunin mentioned this pull request Nov 2, 2015

Require all CRAM files to have a reference #362

Closed

vadimzalunin force-pushed the CRAM_uint32_t_in_ReadTag branch from 7729dc8 to 19e8e3b Compare November 2, 2015 11:09

nh13 reviewed Nov 4, 2015
View reviewed changes

cmnbroad mentioned this pull request Nov 4, 2015

Fixes for CRAM bugs. #368

Merged

vdauwera added the cram label Nov 11, 2015

vadimzalunin force-pushed the CRAM_uint32_t_in_ReadTag branch 3 times, most recently from 78309c8 to 7749f23 Compare November 11, 2015 13:11

ronlevine reviewed Nov 11, 2015
View reviewed changes

droazen force-pushed the CRAM_uint32_t_in_ReadTag branch from 3676253 to fbeb137 Compare November 12, 2015 22:59

fix for unsigned int in CRAM tags, issue #322

ca7e1c9

droazen force-pushed the CRAM_uint32_t_in_ReadTag branch from fbeb137 to ca7e1c9 Compare November 12, 2015 23:15

droazen added a commit that referenced this pull request Nov 12, 2015

Merge pull request #339 from samtools/CRAM_uint32_t_in_ReadTag

9bd6406

unsigned int is backed by a long value

droazen merged commit 9bd6406 into master Nov 12, 2015

droazen deleted the CRAM_uint32_t_in_ReadTag branch November 12, 2015 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unsigned int is backed by a long value #339

unsigned int is backed by a long value #339

vadimzalunin commented Sep 30, 2015

droazen commented Oct 22, 2015

droazen commented Oct 22, 2015

mp15 commented Oct 23, 2015

vadimzalunin commented Oct 23, 2015

droazen commented Oct 23, 2015

vadimzalunin commented Oct 23, 2015

droazen Oct 23, 2015

droazen commented Oct 23, 2015

vadimzalunin commented Oct 28, 2015

vadimzalunin commented Oct 29, 2015

vadimzalunin commented Oct 29, 2015

mp15 commented Nov 2, 2015

nh13 Nov 4, 2015

nh13 commented Nov 4, 2015

droazen commented Nov 6, 2015

vadimzalunin commented Nov 11, 2015

ronlevine Nov 11, 2015

vadimzalunin commented Nov 11, 2015

vadimzalunin commented Nov 12, 2015

droazen commented Nov 12, 2015

droazen commented Nov 12, 2015

droazen commented Nov 12, 2015

vadimzalunin commented Nov 12, 2015

coveralls commented Sep 14, 2017

unsigned int is backed by a long value #339

unsigned int is backed by a long value #339

Conversation

vadimzalunin commented Sep 30, 2015

droazen commented Oct 22, 2015

droazen commented Oct 22, 2015

mp15 commented Oct 23, 2015

vadimzalunin commented Oct 23, 2015

droazen commented Oct 23, 2015

vadimzalunin commented Oct 23, 2015

droazen Oct 23, 2015

Choose a reason for hiding this comment

droazen commented Oct 23, 2015

vadimzalunin commented Oct 28, 2015

vadimzalunin commented Oct 29, 2015

vadimzalunin commented Oct 29, 2015

mp15 commented Nov 2, 2015

nh13 Nov 4, 2015

Choose a reason for hiding this comment

nh13 commented Nov 4, 2015

droazen commented Nov 6, 2015

vadimzalunin commented Nov 11, 2015

ronlevine Nov 11, 2015

Choose a reason for hiding this comment

vadimzalunin commented Nov 11, 2015

vadimzalunin commented Nov 12, 2015

droazen commented Nov 12, 2015

droazen commented Nov 12, 2015

droazen commented Nov 12, 2015

vadimzalunin commented Nov 12, 2015

coveralls commented Sep 14, 2017