Adding in codec to read from Gencode GTF files. Fixes #3277 #3410

jonn-smith · 2017-08-04T15:44:01Z

Includes tests for both hg19 and hg38.

droazen · 2017-08-04T19:06:39Z

@jonn-smith There are a bunch of test failures on this branch in travis -- could you fix these before we review? Thanks!

jonn-smith · 2017-08-04T21:27:36Z

@droazen @cmnbroad @lbergelson

Sorry about that. Fixed the warning. Should be ready for review.

codecov-io · 2017-08-04T21:28:58Z

Codecov Report

Merging #3410 into master will decrease coverage by 0.066%.
The diff coverage is 66.96%.

@@               Coverage Diff               @@
##              master     #3410       +/-   ##
===============================================
- Coverage     79.901%   79.835%   -0.066%     
- Complexity     17918     18315      +397     
===============================================
  Files           1199      1217       +18     
  Lines          65102     67142     +2040     
  Branches       10142     10550      +408     
===============================================
+ Hits           52017     53603     +1586     
- Misses          9045      9329      +284     
- Partials        4040      4210      +170

Impacted Files	Coverage Δ	Complexity Δ
...der/utils/codecs/GENCODE/GencodeGtfCDSFeature.java	`100% <100%> (ø)`	`4 <4> (?)`
...der/utils/codecs/GENCODE/GencodeGtfUTRFeature.java	`100% <100%> (ø)`	`4 <4> (?)`
...odecs/GENCODE/GencodeGtfSelenocysteineFeature.java	`100% <100%> (ø)`	`4 <4> (?)`
...ils/codecs/GENCODE/GencodeGtfStopCodonFeature.java	`100% <100%> (ø)`	`4 <4> (?)`
...ls/codecs/GENCODE/GencodeGtfStartCodonFeature.java	`100% <100%> (ø)`	`4 <4> (?)`
...er/utils/codecs/GENCODE/GencodeGtfExonFeature.java	`46.939% <46.939%> (ø)`	`12 <12> (?)`
...tils/codecs/GENCODE/GencodeGtfFeatureBaseData.java	`48.235% <48.235%> (ø)`	`3 <3> (?)`
...er/utils/codecs/GENCODE/GencodeGtfGeneFeature.java	`53.333% <53.333%> (ø)`	`8 <8> (?)`
...llbender/utils/codecs/GENCODE/GencodeGtfCodec.java	`53.36% <53.36%> (ø)`	`49 <49> (?)`
...ls/codecs/GENCODE/GencodeGtfTranscriptFeature.java	`58.333% <58.333%> (ø)`	`12 <12> (?)`
... and 37 more

droazen · 2017-08-07T14:33:48Z

@jonn-smith Thanks -- will start my review now

droazen

First-pass review complete -- back to @jonn-smith for changes.

Looks really good overall! I might have found a couple of possible bugs here and there, and I had some refactoring/style-related comments as well.

droazen · 2017-08-08T13:02:57Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodec.java

+ * {@link htsjdk.tribble.Tribble} Codec to read data from a GENCODE GTF file.
+ *
+ * GENCODE GTF Files are defined here: https://www.gencodegenes.org/data_format.html
+ *


Document any limitations of the codec in the class-level javadoc. Eg., if tabix indexing doesn't work or hasn't been tested, mention that, and if tribble indexing has any issues, mention those as well.

Also prominently document the fact that this codec, unlike most other tribble codecs, can consume multiple lines at a time in the process of decoding a Feature record. Include a brief overview of the high-level structure of a GTF record.

Sounds good.

droazen · 2017-08-08T13:06:31Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodec.java

+ */
+final public class GencodeGtfCodec extends AbstractFeatureCodec<GencodeGtfFeature, LineIterator> {
+
+    protected final Logger logger = LogManager.getLogger(this.getClass());


We usually make our loggers static final (passing in GencodeGtfCodec.class, in this case) -- unless you can think of a good reason to have a separate logger instance for every codec instance.

Nope - makes sense to me.

droazen · 2017-08-08T13:08:31Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodec.java

+    static final int NUM_COLUMNS = 9;
+
+    private long currentLineNum = 1;
+    protected List<String> header = new ArrayList<>();


Does this have to be protected? There are no subclasses of GencodeGtfCodec, as far as I can tell. Make private if possible, or at least make immutable. Or if it's protected for unit testing purposes, mark as @VisibleForTesting

I think making it private should be fine.

droazen · 2017-08-08T13:12:47Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodec.java

+ *
+ * Created by jonn on 7/21/17.
+ */
+final public class GencodeGtfCodec extends AbstractFeatureCodec<GencodeGtfFeature, LineIterator> {


Document for posterity why this codec extends AbstractFeatureCodec directly rather than the more common AsciiFeatureCodec.

droazen · 2017-08-08T13:28:57Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodec.java

+        GencodeGtfGeneFeature gene = null;
+        GencodeGtfTranscriptFeature transcript = null;
+        final ArrayList< GencodeGtfExonFeature > exonStore = new ArrayList<>();
+        final ArrayList< GencodeGtfFeature > leafFeatureStore = new ArrayList<>();


We typically use the interface type (List) on the left side when creating lists, unless downstream code is explicitly dependent on the List actually being an ArrayList.

Also, on a minor stylistic note, we typically write List<Whatever> rather than List< Whatever >. The latter brings back bad memories of ancient C++ where nested type parameters could get confused with the >> and << operators.

I'm pretty sure I'm not using anything ArrayList-specific. I'll update them to be Lists.

Sounds good. Yeah... Old habits die hard. I've spent a lot of time in C++ land... Sometimes my hands just put the spaces in.

droazen · 2017-08-08T20:45:08Z

src/test/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodecUnitTest.java

+                                new GencodeGtfFeature.OptionalField<>("tag", GencodeGtfFeature.FeatureTag.CCDS),
+                                new GencodeGtfFeature.OptionalField<>("tag", GencodeGtfFeature.FeatureTag.seleno),
+                                new GencodeGtfFeature.OptionalField<>("ccdsid", "CCDS43034.1"),
+                                new Gencod


canDecodeProvider?

droazen · 2017-08-08T20:47:24Z

src/test/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodecUnitTest.java

+                                new GencodeGtfFeature.OptionalField<>("tag", GencodeGtfFeature.FeatureTag.CCDS),
+                                new GencodeGtfFeature.OptionalField<>("tag", GencodeGtfFeature.FeatureTag.seleno),
+                                new GencodeGtfFeature.OptionalField<>("ccdsid", "CCDS43034.1"),
+                                new Gencod


Before doing this assert, assert that expectedIterator.hasNext()

droazen · 2017-08-08T20:48:31Z

src/test/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodecUnitTest.java

+                                new GencodeGtfFeature.OptionalField<>("tag", GencodeGtfFeature.FeatureTag.CCDS),
+                                new GencodeGtfFeature.OptionalField<>("tag", GencodeGtfFeature.FeatureTag.seleno),
+                                new GencodeGtfFeature.OptionalField<>("ccdsid", "CCDS43034.1"),
+                                new Gencod


After the loop, assert that the total number of features returned from the decode() method is equal to expected.size()

droazen · 2017-08-08T20:49:56Z

src/test/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodecUnitTest.java

+                                new GencodeGtfFeature.OptionalField<>("tag", GencodeGtfFeature.FeatureTag.CCDS),
+                                new GencodeGtfFeature.OptionalField<>("tag", GencodeGtfFeature.FeatureTag.seleno),
+                                new GencodeGtfFeature.OptionalField<>("ccdsid", "CCDS43034.1"),
+                                new Gencod


createTempDir() has already registered the directory for recursive deletion on exit, so this is redundant.

droazen · 2017-08-08T20:52:14Z

src/test/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodecUnitTest.java

+                                new GencodeGtfFeature.OptionalField<>("tag", GencodeGtfFeature.FeatureTag.CCDS),
+                                new GencodeGtfFeature.OptionalField<>("tag", GencodeGtfFeature.FeatureTag.seleno),
+                                new GencodeGtfFeature.OptionalField<>("ccdsid", "CCDS43034.1"),
+                                new Gencod


You might want this to test serializeToString() instead. toString() itself should only be for display purposes (no code should rely upon its return value being in a certain format).

magicDGS · 2017-08-09T10:27:08Z

Hi @jonn-smith, I wonder if some part of this code could be ported to a more general extensible GTF codec in HTSJDK to solve samtools/htsjdk#537 and allow the implementation of specifications like GENCODE.

I know that in HTSJDK takes longer to include some new code, but it will be nice to implement here the support for any GTF specification and then port the code in HTSJDK and keeping in GATK only the GENCODE codec.

Thanks in advance!

droazen · 2017-08-09T12:58:31Z

@magicDGS The plan is to eventually push this codec down into htsjdk for wider use by the community, but we want to develop it here first for the sake of fast iteration. Since the GATK project that depends on this branch has a looming deadline, we're unable to generalize this code further at this time, but we can revisit this when the codec is moved into htsjdk.

magicDGS · 2017-08-09T13:02:01Z

Thanks @droazen!

droazen

Final review complete, back to @jonn-smith. Found one bug, but otherwise mostly minor comments remaining. Merge after addressing them -- no need for another review round.

droazen · 2017-08-24T20:03:34Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodec.java

+package org.broadinstitute.hellbender.utils.codecs.GENCODE;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.sun.tools.internal.xjc.reader.xmlschema.bindinfo.BIConversion;


Remove stray com.sun import

droazen · 2017-08-24T20:03:43Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodec.java

+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.broadinstitute.hellbender.exceptions.UserException;
+import scala.tools.nsc.Global;


Remove stray scala import

droazen · 2017-08-24T20:25:34Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodec.java

+                        break;
+                }
+
+                lastRecordWasTranscript = false;


Name of this flag (lastRecordWasTranscript) is confusing, since it's not actually keeping track of whether the previous encountered record was a transcript. Can you rename to reflect what it actually represents?

Sure - I'll call it needToFlushRecords

droazen · 2017-08-24T20:27:54Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodec.java

+        // If we have other records left over we should probably yell a lot,
+        // as this is bad.
+        //
+        // However, this should never actually happen.


Throw a GATKException.ShouldNeverReachHereException in this case instead of a RuntimeException. It's good to keep this sanity check around for now while this is still under development.

Sounds good.

droazen · 2017-08-24T20:30:17Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfCodec.java

+
+        // Now we validate our feature before returning it:
+        if ( ! validateGencodeGtfFeature( decodedFeature, versionNumber ) ) {
+            throw new RuntimeException("Decoded feature is not valid: " + decodedFeature);


UserException.MalformedFile or just UserException. Bad input is the user's fault.

droazen · 2017-08-24T20:53:58Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfExonFeature.java

+
+    public void setCds(GencodeGtfCDSFeature cds) {
+        if (this.cds != null) {
+            throw new RuntimeException("Attempting to set cds, but it already contains a value!");


Use GATKException for errors that are internal (not the user's fault). Applies throughout this PR. (the exception is checking methods arguments, for which IllegalArgumentException is fine)

Yup. Fixed.

droazen · 2017-08-24T20:56:53Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfFeature.java

+    /**
+     * Populate this GencodeGtfFeature with the given data.
+     */
+    protected GencodeGtfFeature(String[] gtfFields) {


Check that gtfFields is the right length using Utils.validateArg()

droazen · 2017-08-24T20:58:50Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfFeature.java

+        baseData.genomicPosition = new SimpleInterval (
+                gtfFields[CHROMOSOME_NAME_INDEX],
+                Integer.valueOf( gtfFields[START_LOCATION_INDEX] ),
+                Integer.valueOf( gtfFields[END_LOCATION_INDEX] )


For all calls to Integer.valueOf(), trap the potential NumberFormatException and throw either a UserException or a GATKException depending on whether or not it was the user's fault.

Sounds good.

droazen · 2017-08-24T21:03:51Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfFeature.java

+        boolean isEqual = that instanceof GencodeGtfFeature;
+        if (isEqual) {
+            GencodeGtfFeature thatFeature = (GencodeGtfFeature) that;
+            Objects.equals(baseData, thatFeature.baseData);


BUGBUG: you need to assign the return value of Objects.equals() back to isEqual!

Yup! Good find!

droazen · 2017-08-24T21:05:58Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfFeature.java

+        }
+
+        @Override
+        public int hashCode() {


Don't use toString() to generate the hashCode() -- toString() is just for display. Take the hashes of the individual fields and combine them, as you've done elsewhere.

I guess I missed this one!

Fixed!

Includes tests for both hg19 and hg38.

Also cleaned up some warnings (e.g. non-final fields became final).

magicDGS · 2017-09-13T15:09:28Z

src/main/java/org/broadinstitute/hellbender/utils/codecs/GENCODE/GencodeGtfFeature.java

     * @return true if {@code other} is contained within the bounds of this {@link GencodeGtfFeature}, false otherwise.
     */
-    public boolean contains(final GencodeGtfFeature other) {
+    public <T extends Locatable> boolean contains(final T other) {


Does this need to be a generic? I think that using public boolean contains(Locatable other) might work...

You might be right - I'll try some tests with it to see.

Yup - I'll pull it back to being a pure Locatable.

jonn-smith requested review from droazen, lbergelson and cmnbroad August 4, 2017 15:44

droazen self-assigned this Aug 4, 2017

droazen suggested changes Aug 8, 2017

View reviewed changes

droazen removed request for lbergelson and cmnbroad August 8, 2017 20:57

droazen assigned jonn-smith and unassigned droazen Aug 8, 2017

jonn-smith force-pushed the jts_GENCODE_tribble_codec_3277 branch from fe41911 to 1291e8a Compare August 11, 2017 19:32

jonn-smith assigned droazen and unassigned jonn-smith Aug 11, 2017

droazen suggested changes Aug 24, 2017

View reviewed changes

droazen assigned jonn-smith and unassigned droazen Aug 24, 2017

jonn-smith force-pushed the jts_GENCODE_tribble_codec_3277 branch from 1291e8a to 92d6c84 Compare August 28, 2017 16:03

jonn-smith added 5 commits September 12, 2017 13:58

Adding in codec to read from Gencode GTF files. Fixes #3277

e63d482

Includes tests for both hg19 and hg38.

Fixing a warning in the test suite.

cd9b953

Addressing the first of the code review comments.

aa89d36

Addressing latest code review comments.

33c2855

Also cleaned up some warnings (e.g. non-final fields became final).

Added in a UCSC version number to GencodeGtfFeature.

28037e4

jonn-smith force-pushed the jts_GENCODE_tribble_codec_3277 branch from a8a9270 to 28037e4 Compare September 12, 2017 17:58

Refactored GencodeGtfFeature::contains to take <T extends Locatable>.

bffea19

magicDGS reviewed Sep 13, 2017

View reviewed changes

jonn-smith added 2 commits September 13, 2017 16:59

Added GencodeGtfFeature::overlaps (leverages SimpleInterval::overlaps).

e2daf9f

Fixing unnecessary generic.

59c18f1

jonn-smith merged commit 4ac5b36 into master Sep 18, 2017

jonn-smith deleted the jts_GENCODE_tribble_codec_3277 branch September 18, 2017 15:00

Adding in codec to read from Gencode GTF files. Fixes #3277 #3410

Adding in codec to read from Gencode GTF files. Fixes #3277 #3410

Conversation

jonn-smith commented Aug 4, 2017

droazen commented Aug 4, 2017

jonn-smith commented Aug 4, 2017

codecov-io commented Aug 4, 2017 • edited Loading

Codecov Report

droazen commented Aug 7, 2017

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

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

magicDGS commented Aug 9, 2017

droazen commented Aug 9, 2017

magicDGS commented Aug 9, 2017

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

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

codecov-io commented Aug 4, 2017 •

edited

Loading