/htsjdk

FeatureCodec.getTabixFormat() to encapsulate tabix formatting #669

Merged
merged 5 commits into from Aug 10, 2016

Conversation

Reviewers
No reviews
Assignees

@yfarjoun yfarjoun

Labels
Review-party candidate
Projects
None yet
Milestone
No milestone
5 participants
@magicDGS @coveralls @droazen @yfarjoun @lbergelson
@magicDGS
Contributor

magicDGS commented Jul 26, 2016 • edited

Description

Current implementation of tabix indexing requires a TabixFormat, which should be defined by the API user outside the codecs except for the static instances inside the class (VCF and BED). Due to this, IndexFactory requires always to provide the TabixFormat when creating a new index, not allowing the creation of indexes with a common interface.

Nevertheless, when developers implements a new FeatureCodec, they know how the format for tabix is specified (if it is possible), and others could use it without the need to implement it by themselves.

Here I implemented a new default method for FeatureCodec for retrieve the TabixFormat associated, and added some static methods to IndexFactory to use it. This could be useful in GATK4 tool IndexFeatureFile, which only supports tabix indexing for VCF files.

It does not break compatibility because the new method have a default implementation, so other classes don't have to implement it.

Checklist

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

magicDGS added some commits Jul 26, 2016

@magicDGS magicDGS added getTabixFormat method to FeatureCodec interface to index files … 
…with tabix when the format is defined in implementors
 7251519
@magicDGS magicDGS changed as a default method in FeatureCodec e241a8e
@magicDGS magicDGS added javadoc param description

Some checks were not successful

1 failing and 1 successful checks
coverage/coveralls — Coverage decreased (-0.01%) to 68.407%
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
afa1838
@coveralls

coveralls commented Jul 26, 2016

Coverage Status

Coverage decreased (-0.01%) to 68.407% when pulling afa1838 on magicDGS:dgs_featurecodec_tabixformat into fcfad25 on samtools:master.
@magicDGS magicDGS added tests and final getTabixFormat for binary codecs

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.03%) to 68.45%
Details
0bc504c
@coveralls

coveralls commented Jul 26, 2016

Coverage Status

Coverage increased (+0.03%) to 68.45% when pulling 0bc504c on magicDGS:dgs_featurecodec_tabixformat into fcfad25 on samtools:master.

droazen self-assigned this Jul 26, 2016

@yfarjoun yfarjoun and 1 other commented on an outdated diff Aug 9, 2016

View full outdated diff
...est/java/htsjdk/variant/vcf/AbstractVCFCodecTest.java
@@ -50,4 +52,10 @@ public void testCanDecodeFile(String potentialInput, boolean canDecode) {
Assert.assertEquals(AbstractVCFCodec.canDecodeFile(potentialInput, VCFCodec.VCF4_MAGIC_HEADER), canDecode);
}
+ @Test
+ public void testGetTabixFormat() {
+ Assert.assertEquals(new VCFCodec().getTabixFormat(), TabixFormat.VCF);
+ Assert.assertEquals(new VCF3Codec().getTabixFormat(), TabixFormat.VCF);
+ }
+
@yfarjoun

yfarjoun Aug 9, 2016

Contributor

remove NL

@magicDGS

magicDGS Aug 10, 2016

Contributor

Done

@yfarjoun yfarjoun and 1 other commented on an outdated diff Aug 9, 2016

View full outdated diff
src/test/java/htsjdk/tribble/bed/BEDCodecTest.java
@@ -221,4 +222,9 @@ private void createIndex(File testFile, File idxFile) throws IOException {
}
@yfarjoun

yfarjoun Aug 9, 2016

Contributor

out of your code, but remove NL

@magicDGS

magicDGS Aug 10, 2016

Contributor

Done

@yfarjoun yfarjoun and 1 other commented on an outdated diff Aug 9, 2016

View full outdated diff
src/main/java/htsjdk/tribble/bed/BEDCodec.java
@@ -224,4 +225,9 @@ public int value() {
}
}
+ @Override
+ public TabixFormat getTabixFormat() {
+ return TabixFormat.BED;
+ }
+
@yfarjoun

yfarjoun Aug 9, 2016

Contributor

remove NL

@magicDGS

magicDGS Aug 10, 2016

Contributor

Done

@yfarjoun yfarjoun and 1 other commented on an outdated diff Aug 9, 2016

View full outdated diff
src/main/java/htsjdk/tribble/BinaryFeatureCodec.java
@@ -40,4 +41,13 @@ public boolean isDone(final PositionalBufferedStream source) {
throw new RuntimeIOException("Failure reading from stream.", e);
}
}
+
+ /**
+ * Marked as final because binary features could not be tabix indexed
+ */
+ @Override
+ public final TabixFormat getTabixFormat() {
+ throw new TribbleException("Binary codecs does not support tabix");
+ }
+
@yfarjoun

yfarjoun Aug 9, 2016

Contributor

remove nl

@magicDGS

magicDGS Aug 10, 2016

Contributor

Done

@yfarjoun yfarjoun and 1 other commented on an outdated diff Aug 9, 2016

View full outdated diff
src/main/java/htsjdk/tribble/AbstractFeatureCodec.java
@@ -47,4 +49,5 @@ public Feature decodeLoc(final SOURCE source) throws IOException {
public Class<FEATURE_TYPE> getFeatureType() {
return myClass;
}
+
@yfarjoun

yfarjoun Aug 9, 2016

Contributor

remove nl

@magicDGS

magicDGS Aug 10, 2016

Contributor

Done

@droazen droazen assigned yfarjoun and unassigned droazen Aug 9, 2016

@lbergelson lbergelson and 1 other commented on an outdated diff Aug 9, 2016

View full outdated diff
src/main/java/htsjdk/tribble/index/IndexFactory.java
@@ -260,11 +260,25 @@ public static LinearIndex createLinearIndex(final File inputFile, final FeatureC
public static <FEATURE_TYPE extends Feature, SOURCE_TYPE> Index createIndex(final File inputFile,
final FeatureCodec<FEATURE_TYPE, SOURCE_TYPE> codec,
final IndexType type) {
+ return createIndex(inputFile, codec, type, null);
+ }
+
+ /**
+ * Create a index of the specified type with default binning parameters
@lbergelson

lbergelson Aug 9, 2016

Contributor

nitpicking: create an index. Typo Police are here!

@magicDGS

magicDGS Aug 10, 2016

Contributor

Thanks! Done

@yfarjoun yfarjoun and 1 other commented on an outdated diff Aug 9, 2016

View full outdated diff
src/main/java/htsjdk/tribble/index/IndexFactory.java
@@ -318,7 +332,18 @@ public static void writeIndex(final Index idx, final File idxFile) throws IOExce
return (TabixIndex)createIndex(inputFile, new FeatureIterator<FEATURE_TYPE, SOURCE_TYPE>(inputFile, codec), indexCreator);
}
-
+ /**
+ * @param inputFile The file to be indexed.
+ * @param codec Mechanism for reading inputFile.
@yfarjoun

yfarjoun Aug 9, 2016

Contributor

" the codec to use for decoding records" is a better description.

@magicDGS

magicDGS Aug 10, 2016

Contributor

Done
@droazen
Contributor

droazen commented Aug 9, 2016

👍 This looks good to me -- let's merge after @yfarjoun's comments are addressed.

droazen referenced this pull request Aug 9, 2016

Open

Review "Party" #548

@yfarjoun
Contributor

yfarjoun commented Aug 9, 2016

nit-picky comments about newlines and javadoc but otherwise LGTM, thanks!

yfarjoun added the Review-party candidate label Aug 9, 2016

@magicDGS magicDGS addressed comments

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — First build on master at 68.807%
Details
a50ee03
@magicDGS
Contributor

magicDGS commented Aug 10, 2016

Back to you, @yfarjoun. If I should squash and rebase let me know.
@coveralls

coveralls commented Aug 10, 2016

Coverage Status

Changes Unknown when pulling a50ee03 on magicDGS:dgs_featurecodec_tabixformat into * on samtools:master*.
@coveralls

coveralls commented Aug 10, 2016

Coverage Status

Changes Unknown when pulling a50ee03 on magicDGS:dgs_featurecodec_tabixformat into * on samtools:master*.
@coveralls

coveralls commented Aug 10, 2016

Coverage Status

Changes Unknown when pulling a50ee03 on magicDGS:dgs_featurecodec_tabixformat into * on samtools:master*.

@yfarjoun yfarjoun merged commit fba4637 into samtools:master Aug 10, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 68.807%
Details
@yfarjoun
Contributor

yfarjoun commented Aug 10, 2016

thanks!

magicDGS referenced this pull request in broadinstitute/gatk Aug 10, 2016

Closed

IndexFeatureFile support for tabix index apart of VCF #2080

@jamesemery jamesemery added a commit to jamesemery/htsjdk that referenced this pull request Sep 1, 2016

@magicDGS @jamesemery magicDGS + jamesemery FeatureCodec.getTabixFormat() to encapsulate tabix formatting (#669) 
* added getTabixFormat method to FeatureCodec interface to index files with tabix when the format is defined in implementors

* changed as a default method in FeatureCodec

* added javadoc param description

* added tests and final getTabixFormat for binary codecs

* addressed comments
 7723019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment