Yf add interval list codec #1327

yfarjoun · 2019-03-15T03:22:21Z

moved the business part of the dec from IntervalList to the Tribble dec.
testing the Tribble directly.

codecov-io · 2019-03-15T03:56:20Z

Codecov Report

Merging #1327 into master will increase coverage by 0.307%.
The diff coverage is 75%.

@@               Coverage Diff               @@
##              master     #1327       +/-   ##
===============================================
+ Coverage     67.822%   68.129%   +0.307%     
- Complexity      8253      8406      +153     
===============================================
  Files            562       563        +1     
  Lines          33644     34260      +616     
  Branches        5640      5778      +138     
===============================================
+ Hits           22818     23341      +523     
- Misses          8654      8720       +66     
- Partials        2172      2199       +27

Impacted Files	Coverage Δ	Complexity Δ
.../java/htsjdk/samtools/util/BufferedLineReader.java	`68.421% <ø> (ø)`	`6 <0> (ø)`	⬇️
...c/main/java/htsjdk/samtools/util/IntervalList.java	`73.897% <100%> (-0.613%)`	`69 <0> (-6)`
src/main/java/htsjdk/samtools/util/Interval.java	`61.29% <33.333%> (-2.995%)`	`28 <1> (+1)`
...main/java/htsjdk/variant/vcf/AbstractVCFCodec.java	`73.846% <60%> (+0.081%)`	`86 <1> (ø)`	⬇️
...htsjdk/tribble/IntervalList/IntervalListCodec.java	`79.31% <79.31%> (ø)`	`14 <14> (?)`
src/main/java/htsjdk/samtools/cram/CRAIIndex.java	`86.813% <0%> (-1.593%)`	`39% <0%> (+13%)`
...a/htsjdk/samtools/cram/build/ContainerFactory.java	`95.122% <0%> (-1.545%)`	`6% <0%> (+1%)`
...jdk/samtools/cram/build/CramContainerIterator.java	`88.889% <0%> (-1.111%)`	`16% <0%> (+6%)`
...java/htsjdk/samtools/cram/structure/Container.java	`90.411% <0%> (-0.065%)`	`42% <0%> (+20%)`
...mtools/cram/build/CramContainerHeaderIterator.java	`100% <0%> (ø)`	`4% <0%> (+2%)`	⬆️
... and 13 more

cmnbroad

I can see why this was painful. A few suggestions and some cleanup comments.

cmnbroad · 2019-03-15T20:08:13Z

src/main/java/htsjdk/tribble/AsciiSamFeatureCodec.java

+ * @param <T> The feature type this codec reads
+ */
+public abstract class AsciiSamFeatureCodec<T extends Feature> extends AbstractFeatureCodec<T, LineReader> {
+    private static final Log log = Log.getInstance(AsciiSamFeatureCodec.class);


cmnbroad · 2019-03-15T21:46:29Z

src/main/java/htsjdk/samtools/util/LineReader.java

-public interface LineReader extends Closeable{
+public interface LineReader extends Closeable {
+
+    int EOF_VALUE = -1;


This needs javadoc, and peek's javadoc should reference it.

cmnbroad · 2019-03-15T21:48:42Z

src/main/java/htsjdk/tribble/AsciiSamFeatureCodec.java

+ *
+ * @param <T> The feature type this codec reads
+ */
+public abstract class AsciiSamFeatureCodec<T extends Feature> extends AbstractFeatureCodec<T, LineReader> {


Sam is in the name but I'm not sure what is particularly Sam about it ?

perhaps I'll use LineReaderFeatureCodec?

cmnbroad · 2019-03-15T21:49:16Z

src/main/java/htsjdk/tribble/AsciiSamFeatureCodec.java

+    }
+
+    @Override
+    public boolean isDone(final LineReader lineIterator) {


These arguments arguments aren' iterators would be better called lineReader rather than lineIterator here and below.

👍 leftover cruft.

cmnbroad · 2019-03-15T21:56:40Z

src/main/java/htsjdk/tribble/IntervalList/IntervalListCodec.java

+
+    static final Log log = Log.getInstance(IntervalListCodec.class);
+
+    final char[] shortArray;


This appears to be write-only...

cmnbroad · 2019-03-16T19:12:12Z

src/main/java/htsjdk/tribble/IntervalList/IntervalListCodec.java

+        // Make sure we have the right number of fields
+        final String[] fields = line.split("\t");
+        if (fields.length != 5) {
+            throw new SAMException("Invalid interval record contains " +


Hm - maybe we've already crossed this line elsewhere (?) but throwing a SAMException from tribble might not be great. Maybe this should be IllegalArgumentException ?

good point. using TribbleException

cmnbroad · 2019-03-16T19:12:57Z

src/main/java/htsjdk/tribble/IntervalList/IntervalListCodec.java

+        if (seq.equals(lastSeq)) {
+            seq = lastSeq;
+        }
+        lastSeq = seq;


I know you just moved this code from elsewhere, but I assume this is just cheap interning ?

this should be in master already.

Yeah, was just curious what is was doing.

cmnbroad · 2019-03-18T12:55:35Z

src/main/java/htsjdk/tribble/IntervalList/IntervalListCodec.java

+ * Also contains the parseing code for the non-tribble parsing of IntervalLists
+ */
+
+public class IntervalListCodec extends AsciiSamFeatureCodec<Interval> {


You could probably eliminate the need for AsciiSamFeatureCodec altogether by using AsciiFeatureCodec as the base class, and writing an anonymous class implementation of LineReader inside of readActualHeader (see below) as a just-in-time bridge to `SamTextCodec'.

not sure what you mean....

maybe I understood...

cmnbroad · 2019-03-18T12:57:38Z

src/main/java/htsjdk/tribble/IntervalList/IntervalListCodec.java

+
+        final SAMFileHeader header = headerCodec.decode(lineReader, "");
+        dictionary = header.getSequenceDictionary();
+        return header;


Suggested change

return header;

@Override

public Object readActualHeader(LineIterator lineIterator) {

final SAMTextHeaderCodec headerCodec = new SAMTextHeaderCodec();

final SAMFileHeader header = headerCodec.decode(new LineReader() {

int lineNo = 0;

@Override

public String readLine() {

lineNo++;

return lineIterator.next();

}

@Override

public int getLineNumber() {

return lineNo;

}

@Override

public int peek() {

return lineIterator.hasNext() ?

lineIterator.peek().charAt(0) :

LineReader.EOF_VALUE;

}

@Override

public void close() { }

}, "IntervalListCodec");

dictionary = header.getSequenceDictionary();

return header;

}

thanks. I tried that first and failed...this is better.

cmnbroad · 2019-03-18T13:07:53Z

src/main/java/htsjdk/samtools/util/IntervalList.java

            if (line == null) {
                return list;
            }

+            final IntervalListCodec intervalListCodec = new IntervalListCodec(dict);


Ideally IntervalListCodec wouldn't have to provide this constructor, since the codec should really get the dictionary from it's input stream directly. Also, the code above that parses out the header lines is redundant with code thats already in the codec; the codec implementation is better because it doesn't require reading the entire header into a string that is then re-parsed. But, I don't see a way to bridge from the BufferedReader to the codec. We might want to think about deprecating fromReader(final BufferedReader in); it would be easy to replace it with a (stream/file/path) overload that could use the codec directly.

I don't think that there's an action item here...please correct me if I'm wrong :-)

- moved the business part of the dec from IntervalList to the Tribble dec. - testing the Tribble directly.

- using Strand class to parse the strand

pshapiro4broad · 2019-03-25T18:00:08Z

src/main/java/htsjdk/tribble/IntervalList/IntervalListCodec.java

+    }
+
+    public IntervalListCodec(final SAMSequenceDictionary dict) {
+        this();


Normally when chaining constructors you want to chain from fewer arguments to more arguments, as the chaining lets you provide default values (or computed values) for arguments that can also be specified. In this case it would be better IMO to make the no-arg constructor call this constructor using this(null); and make this constructor call super(Interval.class);

pshapiro4broad · 2019-03-25T18:00:37Z

src/main/java/htsjdk/tribble/IntervalList/IntervalListCodec.java

+
+public class IntervalListCodec extends AsciiFeatureCodec<Interval> {
+
+    static final Log log = Log.getInstance(IntervalListCodec.class);


Usually the log field is private since no other class has a need to refer to it.

yfarjoun · 2019-03-25T18:15:44Z

put in a indexed test

yfarjoun · 2019-04-01T18:02:56Z

back to you @cmnbroad

cmnbroad

@yfarjoun Only one thing missing that (I think) we discussed last time - can you add an index test, maybe one in TribbleIndexFeatureReaderTest, similar to the others that are there to make sure the header is properly consumed from the stream when querying the new codec with a FeatureReader. Otherwise looks good.

yfarjoun · 2019-04-02T18:02:02Z

@cmnbroad better?

cmnbroad

👍 when tests pass. thx.

yfarjoun requested a review from cmnbroad March 15, 2019 03:22

yfarjoun mentioned this pull request Mar 15, 2019

Write a tribble codec for parsing Picard interval_list format (+ integration tests in GATK) broadinstitute/gatk#5788

Open

cmnbroad self-assigned this Mar 15, 2019

cmnbroad requested changes Mar 18, 2019

View reviewed changes

Yossi Farjoun added 4 commits March 19, 2019 14:16

Implemented a tribble (co)dec for IntervalList.

df6d0c0

- moved the business part of the dec from IntervalList to the Tribble dec. - testing the Tribble directly.

-cleanup

841bad3

- responding to review comments.

6f75f7b

- using Strand class to parse the strand

-rebased, removed commented code

d5e317f

yfarjoun force-pushed the yf_add_intervalListCodec branch from d71944e to d5e317f Compare March 19, 2019 18:20

pshapiro4broad reviewed Mar 25, 2019

View reviewed changes

- responding to review comments

d71b5c7

cmnbroad reviewed Apr 1, 2019

View reviewed changes

Yossi Farjoun added 2 commits April 2, 2019 14:00

- responding to review comments (added test)

ffb6dc5

- spaces

cdd5501

Yossi Farjoun added 2 commits April 2, 2019 15:30

- fixed a typo, moved tests to same file

5ee7723

- fixed a typo, moved tests to same file

0c7b528

cmnbroad approved these changes Apr 2, 2019

View reviewed changes

yfarjoun merged commit 1ece54c into master Apr 3, 2019

yfarjoun deleted the yf_add_intervalListCodec branch April 3, 2019 01:02

cmnbroad mentioned this pull request Apr 12, 2019

Rename test file so it doesn't masquerade as a Picard interval list. broadinstitute/gatk#5879

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yf add interval list codec #1327

Yf add interval list codec #1327

yfarjoun commented Mar 15, 2019

codecov-io commented Mar 15, 2019 •

edited

Loading

cmnbroad left a comment

cmnbroad Mar 15, 2019

cmnbroad Mar 15, 2019

cmnbroad Mar 15, 2019

yfarjoun Mar 19, 2019

cmnbroad Mar 15, 2019

yfarjoun Mar 19, 2019

cmnbroad Mar 15, 2019

yfarjoun Mar 19, 2019

cmnbroad Mar 16, 2019

yfarjoun Mar 19, 2019

cmnbroad Mar 16, 2019

yfarjoun Mar 19, 2019

yfarjoun Mar 19, 2019

cmnbroad Mar 25, 2019

cmnbroad Mar 18, 2019

yfarjoun Mar 19, 2019

yfarjoun Mar 19, 2019

cmnbroad Mar 18, 2019

yfarjoun Mar 19, 2019

cmnbroad Mar 18, 2019

yfarjoun Mar 19, 2019

pshapiro4broad Mar 25, 2019

pshapiro4broad Mar 25, 2019

yfarjoun commented Mar 25, 2019

yfarjoun commented Apr 1, 2019

cmnbroad left a comment

yfarjoun commented Apr 2, 2019

cmnbroad left a comment


		static final Log log = Log.getInstance(IntervalListCodec.class);

		final char[] shortArray;

-        return header;
+    @Override
+    public Object readActualHeader(LineIterator lineIterator) {
+        final SAMTextHeaderCodec headerCodec = new SAMTextHeaderCodec();
+        final SAMFileHeader header = headerCodec.decode(new LineReader() {
+            int lineNo = 0;
+            @Override
+            public String readLine() {
+                lineNo++;
+                return lineIterator.next();
+            }
+            @Override
+            public int getLineNumber() {
+                return lineNo;
+            }
+            @Override
+            public int peek() {
+                return lineIterator.hasNext() ?
+                        lineIterator.peek().charAt(0) :
+                        LineReader.EOF_VALUE;
+            }
+            @Override
+            public void close() { }
+        }, "IntervalListCodec");
+        dictionary = header.getSequenceDictionary();
+        return header;
+    }


		public class IntervalListCodec extends AsciiFeatureCodec<Interval> {

		static final Log log = Log.getInstance(IntervalListCodec.class);

Yf add interval list codec #1327

Yf add interval list codec #1327

Conversation

yfarjoun commented Mar 15, 2019

codecov-io commented Mar 15, 2019 • edited Loading

Codecov Report

cmnbroad 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

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

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

yfarjoun commented Mar 25, 2019

yfarjoun commented Apr 1, 2019

cmnbroad left a comment

Choose a reason for hiding this comment

yfarjoun commented Apr 2, 2019

cmnbroad left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 15, 2019 •

edited

Loading