BetaIntegerCodecTest and bugfixes #1199
Conversation
src/test/java/htsjdk/samtools/cram/encoding/BetaIntegerCodecTest.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1199 +/- ##
==============================================
+ Coverage 68.45% 68.524% +0.074%
- Complexity 8022 8035 +13
==============================================
Files 542 542
Lines 32704 32714 +10
Branches 5530 5534 +4
==============================================
+ Hits 22386 22417 +31
+ Misses 8113 8095 -18
+ Partials 2205 2202 -3
|
src/test/java/htsjdk/samtools/cram/encoding/BetaIntegerCodecTest.java
Outdated
Show resolved
Hide resolved
src/test/java/htsjdk/samtools/cram/encoding/BetaIntegerCodecTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| int[] actual = new int[values.length]; | ||
| try (InputStream is = new ByteArrayInputStream(os.toByteArray())) { | ||
| BitInputStream bis = new DefaultBitInputStream(is); |
There was a problem hiding this comment.
This should declare bis in the try (...) as well, since it's a closeable resource.
There was a problem hiding this comment.
bis is not AutoCloseable (we should fix that but not today) but I can make it a DefaultBitInputStream which is.
src/test/java/htsjdk/samtools/cram/encoding/BetaIntegerCodecTest.java
Outdated
Show resolved
Hide resolved
src/test/java/htsjdk/samtools/cram/encoding/BetaIntegerCodecTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@jmthibault79 Your changes look good. I have some refactoring suggestions and some things that I think also probably be error cases. If you want to keep this as simple bugfix PR I can do my refactoring suggestions in another branch. Class / constructor javadoc would be really useful here since I had no idea what the point of this was without reading the code and looking at the cram spec.
| @@ -47,8 +47,8 @@ public final long write(final BitOutputStream bitOutputStream, final Integer val | |||
|
|
|||
| @Override | |||
| public final long numberOfBits(final Integer value) { | |||
| if (value > (1L << readNofBits)) | |||
| throw new IllegalArgumentException("Value written is bigger then allowed: value=" + value | |||
| if (value + offset >= (1L << readNofBits)) | |||
There was a problem hiding this comment.
This seems like the correct case. Do we not have any code that actually sets an offset so we never noticed the bug?
It seems like there needs to be an additional constrain here which is value + offset >= 0
There was a problem hiding this comment.
There were 2 errors here: in addition to the offset case, the > meant we allowed e.g. 0x100 for 8 bits. I don't know whether we would have hit these cases before.
It's not clear to me that we should be disallowing negative values here.
There was a problem hiding this comment.
Considering this more thoroughly: yes, this codec is intended to store only positive values (though the source data series may be negative). I'll add a check, a test, and javadoc about this.
src/test/java/htsjdk/samtools/cram/encoding/BetaIntegerCodecTest.java
Outdated
Show resolved
Hide resolved
src/test/java/htsjdk/samtools/cram/encoding/BetaIntegerCodecTest.java
Outdated
Show resolved
Hide resolved
src/main/java/htsjdk/samtools/cram/encoding/BetaIntegerCodec.java
Outdated
Show resolved
Hide resolved
src/main/java/htsjdk/samtools/cram/encoding/BetaIntegerCodec.java
Outdated
Show resolved
Hide resolved
src/main/java/htsjdk/samtools/cram/encoding/BetaIntegerCodec.java
Outdated
Show resolved
Hide resolved
src/main/java/htsjdk/samtools/cram/encoding/BetaIntegerCodec.java
Outdated
Show resolved
Hide resolved
jmthibault79
force-pushed
the
jt_BetaIntegerCodecTest
branch
from
ab500ad
to
1af11ff
Compare
|
Ready for re-review @lbergelson |
There was a problem hiding this comment.
@jmthibault79 This looks good now. Thanks for making those changes, I think it's easier to understand and more robust now.
* allow BetaIntegerEncoding to be specified with 0 bits.
This was disabled in #1199, as a probable error state, but it turns out that it's used in some crams created with htslib.
Checklist