Fix bugs in IndexFactory and BinningIndexBuilder. #906

cmnbroad · 2017-06-20T22:25:21Z

Description

Fixes #393 and a few other issues when creating indices through IndexFactory.create*Index methods that take a file:

Block gzipped inputs were wrapped with PositionalBufferedStreams, resulting in byte offsets being handed to the indexer rather than virtual file pointers.
The first feature was always indexed as if it were at offset 0, even if it was preceded by a header. Most of the time this worked, since the feature iterator thats used to read the indexed file would scan right through the header. But it failed when the header was large (i.e. hg38 large), since the scanner would be short-circuited by the linear index before the header was exhausted.
The BinningIndexBuilder assumed that 0 was never a legitimate feature location, but a file with no header at all (such as a header-less BED file) can have features at offset/virtual-file-pointer 0, even if gzipped.

Checklist

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

codecov-io · 2017-06-20T22:36:25Z

Codecov Report

Merging #906 into master will increase coverage by 0.196%.
The diff coverage is 80%.

@@              Coverage Diff               @@
##              master     #906       +/-   ##
==============================================
+ Coverage     65.564%   65.76%   +0.196%     
- Complexity      7374     7401       +27     
==============================================
  Files            528      529        +1     
  Lines          31981    31968       -13     
  Branches        5467     5463        -4     
==============================================
+ Hits           20968    21022       +54     
+ Misses          8868     8801       -67     
  Partials        2145     2145

Impacted Files	Coverage Δ	Complexity Δ
...sjdk/tribble/readers/PositionalBufferedStream.java	`53.465% <ø> (ø)`	`24 <0> (ø)`	⬇️
...n/java/htsjdk/tribble/index/tabix/TabixFormat.java	`54.286% <ø> (ø)`	`6 <0> (ø)`	⬇️
src/main/java/htsjdk/tribble/FeatureCodec.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...va/htsjdk/tribble/TribbleIndexedFeatureReader.java	`67.222% <ø> (-1.111%)`	`22 <0> (ø)`
...n/java/htsjdk/tribble/readers/AsciiLineReader.java	`84.444% <100%> (+42.339%)`	`17 <4> (+3)`	⬆️
.../samtools/reference/FastaSequenceIndexCreator.java	`61.818% <100%> (ø)`	`8 <7> (ø)`	⬇️
...rc/main/java/htsjdk/tribble/AsciiFeatureCodec.java	`100% <100%> (+16.667%)`	`9 <2> (+1)`	⬆️
...main/java/htsjdk/samtools/BinningIndexBuilder.java	`85.965% <66.667%> (+0.251%)`	`15 <0> (ø)`	⬇️
...c/main/java/htsjdk/tribble/index/IndexFactory.java	`75.817% <70.833%> (-2.85%)`	`26 <0> (ø)`
src/main/java/htsjdk/tribble/bed/BEDCodec.java	`80.233% <75%> (-0.78%)`	`25 <5> (+3)`
... and 12 more

magicDGS

Thanks for doing this @cmnbroad! I learned a lot from this fix. Some minor comments and some the stream problem with my suggestions.

Looking forward to get this in and retrieve the tribble/tabix indexing support in htsjdk and derived projects!

magicDGS · 2017-06-22T08:54:39Z

src/main/java/htsjdk/tribble/readers/AsciiLineReader.java

    public AsciiLineReader(final InputStream is){
+        // NOTE: This will wrap the input stream in a PositionalBufferedStream even if its already a PositionalBufferedStream


Maybe this constructor could be safer by using a private method to wrap the input stream:

private PositionalBufferedStream wrapStream(final InputStream is) { if (is instanceof PositionalBufferedStream) { return (PositionalBufferedStream) is; } else if (is instanceof BlockCompressedInputStream) { throw new IllegalArgumentException("Cannot use AsciiLineReader with BlockCompressedInputStream; use BlockCompressedAsciiLineReader instead"); } else { return new PositionalBufferedStream(is); } }

Not sure it should be illegal to pass a BCS to AsciiLineReader (my comment that it violates the contract might be an overstatement). Perhaps we should just change the doc to be more explicit about the fact that it provides buffering, which affects the positions returned.

magicDGS · 2017-06-22T08:59:58Z

src/main/java/htsjdk/tribble/AsciiFeatureCodec.java

-        if (bufferedInputStream instanceof PositionalBufferedStream) {
-            pbs = (PositionalBufferedStream) bufferedInputStream;
+    public LocationAware makeIndexableSourceFromStream(final InputStream inputStream) {
+        if (inputStream instanceof BlockCompressedInputStream) {


I propose to deal with this in a new static method AsciiLineReader.from(InputStream is) and deprecate the constructor. This will be safer for the users of AsciiLineReader and will simplify the code here.

It seems like that would put an unnecessary constraint on AsciiLineReader (it should be legal to wrap an AsciiLineReader around a BCS). Having said that, its confusing because AsciiLineReader confers both a line orientation, as well as buffering, which is what messes up the positions. This would be a lot simpler if the buffering were separate.

Also, given how brittle all this code is, I like the idea of having this behavior and the comments explicitly included right here, where the explicit control over the buffering/positioning matters (this method is specifically to support indexing).

Since we had to duplicate this code in GATK anyway, I added a "from" method per this suggestion.

magicDGS · 2017-06-22T09:01:14Z

src/main/java/htsjdk/tribble/readers/BlockCompressedAsciiLineReader.java

+ * An AsciiLineReader implementation that wraps a BlockCompressedInputStream and provides no additional buffering.
+ * Useful for cases where we need to preserve virtual file pointers in the underlying stream, such as during indexing.
+ */
+public class BlockCompressedAsciiLineReader extends AsciiLineReader {


If the static method is done in AsciiLineReader, this could be a private inner class to avoid direct usages.

I think now that we have that static method we should make this either a package protected class or an inner class of AsciiLineReader. I don't think we want to expose it unless we have a use case for that.

magicDGS · 2017-06-22T09:04:12Z

src/main/java/htsjdk/tribble/readers/PositionalBufferedStream.java

@@ -28,7 +28,11 @@

 /**
 * A wrapper around an {@code InputStream} which performs it's own buffering, and keeps track of the position.
- * 
+ *
+ * TODO: This class implements Positional, which in turn extends LocationAware, which requires preservation of


This could be fixed by throwing an IllegalArgumentException if the InputStream is a BlockCompressedInputStream as a temporary solution. It is much better than violate the contract and assume that it is working as expected...

I agree that would be the right thing to do, but htsjdk is kind of painted into a corner at this point. There are several other code paths that would have to change, but worse, BinaryFeatureCodec (and thus BCF2Codec) use PositionalBufferedStream for a SOURCE type parameter, even though BCF can be block compressed. So that codec actually depends on being able to wrap a PositionalBufferedStream around a block compressed stream.

magicDGS · 2017-06-22T09:12:37Z

src/test/java/htsjdk/tribble/index/IndexTest.java

@@ -92,6 +88,7 @@ public void testWriteIndex(final File inputFile, final IndexFactory.IndexType ty
        Assert.assertTrue(tempIndex.exists());
        // load the generated index
        final Index loadedIndex = IndexFactory.loadIndex(tempIndex.getAbsolutePath());
+        //TODO: This not very useful; the test can pass even if the generated index is unuseable for queries.


I implemented this test only for checking if writing the index does not blow up, and if the in-memory index and the one written have the same contents. For testing if it is usable for queries, it should be a different test (which actually does not require to write the index down, I guess).

magicDGS · 2017-06-22T09:14:35Z

src/test/java/htsjdk/tribble/index/tabix/TabixIndexTest.java

+    }
+
+    @Test(dataProvider = "bedTabixIndexTestData")
+    public void testBedTabixIndex(


magicDGS · 2017-06-22T09:18:00Z

src/test/java/htsjdk/tribble/index/tabix/TabixIndexTest.java

@@ -102,6 +114,8 @@ public void testQueryProvidedItemsAmount() throws IOException {
        ) // create TabixIndex straight from plaintext VCF
                .write(plainTextVcfIndexFile); // write it

+        //TODO: you can pass in a .tbi file as the index for a plain .vcf, but if you *don't* pass in the file name and


But is it correct to use a .tbi index with a plain text file? As you noted in a previous comment, this is not allowed by tabix... I suggest to blow up in this case!

droazen

Did an initial review pass -- haven't looked at the tests yet, so a second pass will be needed.

droazen · 2017-07-10T19:23:41Z

src/main/java/htsjdk/tribble/AsciiFeatureCodec.java

        } else {
-            pbs = new PositionalBufferedStream(bufferedInputStream);
+            return new AsciiLineReaderIterator(new AsciiLineReader(inputStream));


What about this case? Seems like you're not "indexable" if you reach this else clause.

Possibly; it depends on the type of the inputStream. I left this in since it represents the previous behavior for this method. If inputStream isn't wrapping a block compressed stream, and reports it's position correctly, I think it would work. We could throw instead, but if we we're going to do that, we could deprecate this method and replace it method with two overloads with the correct stream argument types.

It seems like we should have a test case for this case.

Should there at least be a warning if we reach this? It seems very possible that you're going to try indexing stdin or something like that here.

droazen · 2017-07-10T19:39:09Z

src/main/java/htsjdk/tribble/readers/BlockCompressedAsciiLineReader.java

+ * An AsciiLineReader implementation that wraps a BlockCompressedInputStream and provides no additional buffering.
+ * Useful for cases where we need to preserve virtual file pointers in the underlying stream, such as during indexing.
+ */
+public class BlockCompressedAsciiLineReader extends AsciiLineReader {


This class needs unit tests

droazen · 2017-07-10T19:39:35Z

src/main/java/htsjdk/tribble/readers/BlockCompressedAsciiLineReader.java

+    }
+
+    /**
+     * Read a single line of inpu, advnced the underlying stream only enough to read the line.


inpu -> input
advnced -> advanced

droazen · 2017-07-10T19:39:59Z

src/main/java/htsjdk/tribble/readers/BlockCompressedAsciiLineReader.java

+    };
+
+    public String readLine(final PositionalBufferedStream stream) {
+        throw new IllegalArgumentException("A BlockCompressedAsciiLineReader class cannot be used to read from a PositionalBufferedStream");


UnsupportedOperationException

droazen · 2017-07-10T19:41:36Z

src/main/java/htsjdk/samtools/BinningIndexBuilder.java

@@ -50,6 +52,10 @@
     */
    public BinningIndexBuilder(final int referenceSequence, final int sequenceLength) {
        this.referenceSequence = referenceSequence;
+        // Initially set each window to -1 so we can distinguish between windows that have no overlapping
+        // features, and those who's lowest offset is 0, which is a valid (virtual file) offset for feature


who's -> whose

droazen · 2017-07-10T19:44:49Z

src/main/java/htsjdk/samtools/BinningIndexBuilder.java

@@ -33,6 +34,7 @@
 * Builder for a BinningIndexContent object.
 */
 public class BinningIndexBuilder {
+    private static final int UNINITIALIZED_WINDOW = -1;


Can you add a unit test for this class that tests with a record at position 0.

Done. There were previously no unit tests so now there are tests for a few basic cases.

droazen · 2017-07-10T19:45:19Z

src/main/java/htsjdk/tribble/TribbleIndexedFeatureReader.java

@@ -243,6 +243,7 @@ private void readHeader() throws IOException {
        try {
            is = ParsingUtils.openInputStream(path, wrapper);
            if (hasBlockCompressedExtension(new URI(URLEncoder.encode(path, "UTF-8")))) {
+                // TODO: TEST/FIX THIS!


Can you ensure that a github ticket is filed for this?

droazen · 2017-07-10T19:47:11Z

src/main/java/htsjdk/tribble/bed/BEDCodec.java

+     * feature query (otherwise the feature reader can terminate prematurely if the
+     * header is large).
+     * @param lineIterator
+     * @return


Fill in @return

droazen · 2017-07-10T19:47:22Z

src/main/java/htsjdk/tribble/bed/BEDCodec.java

    @Override
-    public Object readActualHeader(LineIterator reader) {
+    public Object readActualHeader(final LineIterator lineIterator) {


Add unit test for this method

droazen · 2017-07-10T19:51:04Z

src/main/java/htsjdk/tribble/AsciiFeatureCodec.java

-        final PositionalBufferedStream pbs;
-        if (bufferedInputStream instanceof PositionalBufferedStream) {
-            pbs = (PositionalBufferedStream) bufferedInputStream;
+    public LocationAware makeIndexableSourceFromStream(final InputStream inputStream) {


Can you file a ticket against LocationAware to either make all implementation obey the interface contract, or change the interface to reflect the actual behavior?

cmnbroad · 2017-07-24T22:29:22Z

This also PR fixes #943, which is a tracking ticket for the BinningIndexBuilder issue fixed as part of this PR.

cmnbroad · 2017-07-25T20:11:02Z

Round 1 done. Back to @droazen.

lbergelson · 2017-07-31T19:28:16Z

src/main/java/htsjdk/tribble/index/IndexFactory.java

+                return new PositionalBufferedStream(fileStream);
+            } catch (final FileNotFoundException e) {
+                throw new TribbleException.FeatureFileDoesntExist("Unable to open the input file, most likely the file doesn't exist.", inputFile.getAbsolutePath());
+            } catch (final IOException e) {


lets make sure we don't get npr in these exceptions

lbergelson · 2017-07-31T19:43:44Z

src/test/java/htsjdk/tribble/index/IndexFactoryTest.java

+            final File inputVCF,
+            final Interval queryInterval) throws IOException {
+        // copy the original file and create the index for the copy
+        final File tmpVCF = new File(IOUtil.createTempDir("testCreateTabixIndexFromVCF", null), inputVCF.getName());


cleanup this temp dir afterwards

lbergelson · 2017-07-31T19:46:43Z

src/test/java/htsjdk/tribble/index/IndexFactoryTest.java

+    @Test(dataProvider = "bcfDataFactory")
+    public void testCreateLinearIndexFromBCF(final File inputBCF) throws IOException {
+        // copy the original file and create the index for the copy
+        final File tmpBCF = new File(IOUtil.createTempDir("testCreateIndexFromBCF", null), inputBCF.getName());


same cleanup issue here I think

lbergelson · 2017-07-31T19:49:43Z

src/test/java/htsjdk/tribble/index/IndexFactoryTest.java

+        index.writeBasedOnFeatureFile(tmpBCF);
+
+        try(final VCFFileReader originalReader = new VCFFileReader(inputBCF,false);
+            final VCFFileReader tmpReader = new VCFFileReader(tmpBCF, new File(tmpBCF.getAbsolutePath()+ ".idx"),true)) {


hardcoded file extension

lbergelson · 2017-07-31T19:55:15Z

src/test/java/htsjdk/tribble/index/tabix/TabixIndexTest.java

+            final List<Interval> queryIntervals
+    ) throws Exception {
+        // copy the input file and create an index for the copy
+        final File tmpBed = new File(IOUtil.createTempDir("testBedTabixIndex", null), inputBed.getName());


temp dirs all over the place

lbergelson

👍 This looks good to me modulo the few minor issues. Merge when those are addressed.

pshapiro4broad · 2017-08-01T15:41:55Z

src/test/java/htsjdk/tribble/index/IndexFactoryTest.java

+        final TabixIndex tabixIndexGz = IndexFactory.createTabixIndex(tmpVCF, new VCFCodec(), null);
+        tabixIndexGz.writeBasedOnFeatureFile(tmpVCF);
+
+        try(final VCFFileReader originalReader = new VCFFileReader(inputVCF,false);


need space after try, and while below.

pshapiro4broad · 2017-08-01T15:42:36Z

src/test/java/htsjdk/tribble/bed/BEDCodecTest.java

+        BEDCodec bedCodec = new BEDCodec();
+        try (final InputStream is = ParsingUtils.openInputStream(gzippedBedFile.getPath());
+             final BlockCompressedInputStream bcis = new BlockCompressedInputStream(is))
+        {


open { should go on previous line, unless the line is too long

For multiline resource blocks I think this way is clearer.

A matter of taste :)
One data point, oracle's Java documentation: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

pshapiro4broad · 2017-08-01T15:43:12Z

src/test/java/htsjdk/tribble/index/IndexFactoryTest.java

+    @DataProvider(name = "bedDataProvider")
+    public Object[][] getLinearIndexFactoryTypes(){
+        return new Object[][] {
+                new Object[] { new File(TestUtils.DATA_DIR, "bed/Unigene.sample.bed") },


unnecessary new Object[], and there shouldn't be a space around the contents of an array initializer, e.g.

public Object[][] getLinearIndexFactoryTypes(){ return new Object[][] { {new File(TestUtils.DATA_DIR, "bed/Unigene.sample.bed")}, {new File(TestUtils.DATA_DIR, "bed/Unigene.sample.bed.gz")} }; }

pshapiro4broad · 2017-08-01T17:54:54Z

src/test/java/htsjdk/tribble/index/IndexFactoryTest.java

-    final File discontinuousFile = new File(TestUtils.DATA_DIR + "bed/disconcontigs.bed");
-    final BEDCodec bedCodec = new BEDCodec();
+    @DataProvider(name = "bedDataProvider")
+    public Object[][] getLinearIndexFactoryTypes(){


missing space before {

pshapiro4broad · 2017-08-01T17:57:57Z

src/test/java/htsjdk/tribble/index/IndexTest.java

-//        for ( int mid = 0; mid <= end; mid += 1000000 ) {
-//            tests.add(new Object[]{0, mid, mid+1000000, end});
-//        }
+        List<Object[]> tests = new ArrayList<>();


It would be shorter to write:

return new Object[][] {{1226943, 1226943, 1226943, 2000000}};

pshapiro4broad · 2017-08-01T18:00:10Z

src/test/java/htsjdk/samtools/BinningIndexBuilderTest.java

+
+public class BinningIndexBuilderTest extends HtsjdkTest {
+
+    static int referenceSequenceIndex = 19;


should be private, final and all uppercase, e.g. REFERENCE_SEQUENCE_INDEX

cmnbroad · 2017-08-03T21:08:43Z

@jrobinso We're planning to make some changes to the way indexing works in order to fix several bugs, and wanted to get out in front of any issues that might arise for IGV. Amongst other things, with this PR, when indexing we no longer close the input SOURCE/stream after reading the header (this affected indexing for at least one codec in GATK). Can you take a look and see if this will require any IGV changes ? Thx.

jrobinso · 2017-08-07T23:58:21Z

I don't see any IGV impact, and if there are I will deal with it in IGV.

lbergelson · 2017-08-08T21:05:31Z

src/main/java/htsjdk/tribble/readers/AsciiLineReader.java

-    public final String readLine(final PositionalBufferedStream stream) throws IOException{
+    // TODO: This should be deprecated and made private/merged with readLine, or made
+    // static, as it reads from the provided input stream rather than the wrapped input stream
+    @Deprecated


missed this before, but could you add a deprecation date?

and a javadoc comment

I put an @deprecated inside the javadoc, with the date, and added an @see to the "from" method. I can't figure out how to anchor to the OTHER readLine method though. Maybe I could if I swapped the order of the methods....

lbergelson · 2017-08-08T21:11:48Z

src/main/java/htsjdk/tribble/readers/AsciiLineReader.java

+     * TODO: This class implements LocationAware, which requires preservation of virtual file pointers on BGZF inputs.
+     * However, if the inputStream wrapped by this class is a BlockCompressedInputStream, it violates that contract by
+     * wrapping the stream and returning positional file offsets instead.
+     */
    public AsciiLineReader(final InputStream is){


Should we deprecate this constructor and eventually make it private? Is there a case outside of this class that we would want to call this constructor instead of from()?

Hm. We could do that. There are a bunch of internal uses of it, but thats easily fixed. But should we deprecate the other one too (the one that takes a PositionalBufferedInputStream), since "from" is the preferred method ? Otherwise its weird to leave the PBS constructor, but not have a BCS constructor (you'd have to use .from for that case). It would be most consistent to just have .from, but thats less BWC.

I think you're right that it would make sense to also deprecate the other one. What's BWC stand for?

Sorry...backward compatible. I'll deprecate the other one as well.

lbergelson

@cmnbroad A few last minute comments. Sorry!

cmnbroad · 2017-08-08T22:12:41Z

@lbergelson I may have added more questions than answers... See what you think.

droazen · 2017-08-09T13:04:24Z

@cmnbroad Before merging this, can we ensure that there are clear and complete instructions for codec authors on how to write a codec that is properly indexable? (eg., in a comment to FeatureCodec)

cmnbroad · 2017-08-09T14:18:45Z

@droazen Done. We should let @lbergelson have one more look at the previous set of changes I made when he gets to it.

lbergelson · 2017-08-09T15:08:45Z

@cmnbroad Looks good to me. 👍 Merge when ready.

cmnbroad · 2017-08-09T15:13:49Z

Squashed and rebase once more. Will merge when tests pass.

pshapiro4broad · 2017-08-09T15:09:35Z

src/main/java/htsjdk/tribble/readers/AsciiLineReader.java

+    protected AsciiLineReader() {};
+
+    /**
+     * TODO: This class implements LocationAware, which requires preservation of virtual file pointers on BGZF inputs.


I don't think you want TODO here. This sounds more like a warning or a note to the user of the class.

Well, it is a TODO, but you're right the TODO doesn't need to be in the javadoc, just the warning.

pshapiro4broad · 2017-08-09T15:16:32Z

src/main/java/htsjdk/tribble/readers/AsciiLineReader.java

+     * However, if the inputStream wrapped by this class is a BlockCompressedInputStream, it violates that contract by
+     * wrapping the stream and returning positional file offsets instead.
+     *
+     * @deprecated 8/8/2017 see {@link #from}


I think the format you want here is use {@link #from(InputStream)}. Saying "see" to me sounds like @see which has a different usage in javadoc. You can provide the types of the parameters if you want to specify an overridden method.

Also it's not great form to use a date, since the date when the software released is not yet fixed, or easy for a user to determine given a .jar they've got lying around. If you want to include this information, use the version number.

I changed the "see"s to "use". AFACIT, we've been putting deprecation dates, I think mostly for our own benefit, since we have existing deprecated stuff but we don't know when it was deprecated. As soon as the maintainer groups defines the deprecation rules, we'll need that info...

We've started adding the dates so we can get a rough sense of how long something has been deprecated. We could always look in the blame which is more accurate, but it's a pain if you're scanning through lots of things. We could provide the version number, but our deprecation removal policy (which doesn't really exist in any official form) is "has this been deprecated for a long time" rather than "has this been deprecated for N versions".

@lbergelson I left the date in - any input on that ? Phil is probably right that its of limited use to the user, but not sure if you guys have discussed the plan for deprecation.

Never mind - just saw your comment....

@cmnbroad Yeah, it's really not intended for the users except for them to say "oh, yeah, this is ancient and I shouldn't use it." A version number wouldn't be a bad idea, but then we have to be sure we guess what version the commit is going into, which is awkward. Checking the blame is the ultimate guide, but it's a pain to do in bulk.

pshapiro4broad · 2017-08-09T15:21:10Z

src/main/java/htsjdk/tribble/readers/BlockCompressedAsciiLineReader.java

+    /**
+     * Read a single line of input, advance the underlying stream only enough to read the line.
+     */
+    public String readLine() throws IOException {


If these are overridden, they need an @Override annotation. Also there's no need to javadoc an overridden method, as javadoc will copy down the base class method doc.

Good catch.

pshapiro4broad · 2017-08-09T15:27:19Z

src/main/java/htsjdk/tribble/readers/AsciiLineReader.java

    private char[] lineBuffer;
    private int lineTerminatorLength = -1;

+    protected AsciiLineReader() {};


This is not a great use of an empty constructor. If subclassed improperly a user ends up with a class that's in an invalid state. It will only work correctly if certain methods are overridden, which are not part of a documented API.

The proper solution is to create an abstract base class (or interface with default methods) that both AsciiLineReader and BlockCompressedAsciiLineReader inherit from. That makes it explicit which methods must be overridden and prevents having inherited fields in BlockCompressedAsciiLineReader that are invalid.

Agreed thats a better solution, but it its not source compatible. We recognize that we need to do something more comprehensive to fix the underlying problems, and there are a couple of issue tickets to represent that. Ideally we'll only break compatibility once for these issues.

cmnbroad mentioned this pull request Jun 20, 2017

Failing test for BED tabix indexing #820

Closed

5 tasks

droazen self-requested a review June 21, 2017 02:04

droazen self-assigned this Jun 21, 2017

magicDGS reviewed Jun 22, 2017

View reviewed changes

droazen requested changes Jul 10, 2017

View reviewed changes

cmnbroad mentioned this pull request Jul 24, 2017

BinningIndexBuilder assumes no feature can be at offset 0 #943

Closed

cmnbroad force-pushed the cn_index_fixes branch from af554f4 to 50747b9 Compare July 25, 2017 18:02

lbergelson reviewed Jul 31, 2017

View reviewed changes

lbergelson approved these changes Jul 31, 2017

View reviewed changes

pshapiro4broad reviewed Aug 1, 2017

View reviewed changes

droazen assigned cmnbroad and unassigned droazen Aug 1, 2017

cmnbroad mentioned this pull request Aug 2, 2017

Upgrade to htsjdk 2.11.0. Make TargetCodec indexable. broadinstitute/gatk#3403

Merged

cmnbroad force-pushed the cn_index_fixes branch from a7a4eb5 to 74e64ff Compare August 8, 2017 18:50

lbergelson reviewed Aug 8, 2017

View reviewed changes

cmnbroad force-pushed the cn_index_fixes branch from 8eb7f93 to 6a7fe97 Compare August 9, 2017 15:13

pshapiro4broad reviewed Aug 9, 2017

View reviewed changes

cmnbroad force-pushed the cn_index_fixes branch from 6a7fe97 to 1cc12f9 Compare August 9, 2017 16:10

Fix bugs in IndexFactory and BinningIndexBuilder.

bebc5ae

cmnbroad force-pushed the cn_index_fixes branch from 1cc12f9 to bebc5ae Compare August 9, 2017 19:29

cmnbroad merged commit 1c44869 into samtools:master Aug 9, 2017

cmnbroad mentioned this pull request Aug 14, 2017

TableCodec isn't properly indexable due to buffering done by OpenCSV reader. broadinstitute/gatk#3440

Open

cmnbroad mentioned this pull request Aug 24, 2017

Re-enable BED test in IndexFeatureFile. broadinstitute/gatk#3507

Merged

		public AsciiLineReader(final InputStream is){
		// NOTE: This will wrap the input stream in a PositionalBufferedStream even if its already a PositionalBufferedStream


		public class BinningIndexBuilderTest extends HtsjdkTest {

		static int referenceSequenceIndex = 19;

Fix bugs in IndexFactory and BinningIndexBuilder. #906

Fix bugs in IndexFactory and BinningIndexBuilder. #906

Conversation

cmnbroad commented Jun 20, 2017 • edited

Description

Checklist

codecov-io commented Jun 20, 2017 • edited

Codecov Report

magicDGS 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

cmnbroad Jun 22, 2017 • edited

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

Choose a reason for hiding this comment

droazen 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

lbergelson Jul 31, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad Jul 25, 2017 • edited

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

Choose a reason for hiding this comment

cmnbroad commented Jul 24, 2017 • edited

cmnbroad commented Jul 25, 2017

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

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

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

cmnbroad commented Aug 3, 2017 • edited

jrobinso commented Aug 7, 2017

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

cmnbroad commented Jun 20, 2017 •

edited

codecov-io commented Jun 20, 2017 •

edited

cmnbroad Jun 22, 2017 •

edited

lbergelson Jul 31, 2017 •

edited

cmnbroad Jul 25, 2017 •

edited

cmnbroad commented Jul 24, 2017 •

edited

cmnbroad commented Aug 3, 2017 •

edited