Tribble writing support #822

magicDGS · 2017-03-10T12:16:59Z

Description

As described in #701, it will be nice to have a simple mechanism to write features in tribble not only for VariantContext. This is a port from one of my project that I think that a lot of API users will benefit. It includes the following:

FeatureWriter interface to encapsulate adding features and writing headers. There is no support for comments yet, but it could be something added in the future.
FeatureEncoder interface to encapsulate feature-specific output formats. Based on a very nice suggestion from @lindenb in one of my previous PRs, the purpose of this interface is to format the feature/header and add it to an Appendable. This allow to write directly the features without storing the String format, and give more flexibility. It also have a method to encapsulate the tabix format, to allow indexing on the fly.
Example of FeatureEncoder to convert any Feature into a simple BED format (tab-delimited chromosome, start and end). It will be used also in tests.
Package protected implementation of FeatureWriter similar to the VCF writer, which uses an encoder and an output stream to create the feature file. This class can be extended for other purposes as noted bellow.
Package protected sub-class of FeatureWriterImpl to index on the fly. This one will require eventually Tribble/Tabix index path support #810 for use the java.nio.Path index method, but it is very useful for writing feature files already indexed.
Wrapper for FeatureWriter to use asynchronous writing. This is a very simple wrapper extending the AbstractAsyncWriter. The only flaw is that the header could not be written asynchronously with the current implementation (and thus, it's not safe). I do not really need that, and if it is a problem that it's not safe, this support could be removed.
Finally, for construction of feature writers, I decided to use a factory pattern instead of the static methods used in the tribble reader classes. This factory have support: 1) index on the fly, 2) block-gzip or gzip, 3) generate MD5 diggest files, 4) asynchronous wriitng and 5) sequence dictionary addition.

All the writing support use java.nio.Path to allow other filesystems. There are no tests for the writers yet (only for the simple encoder), because I want to know your opinion about this addition beforehand. I know that this is a very big PR, but I think that it may be useful for others and will be nice to included in HTSJDK for easier development of new bioinformatic tools.

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 · 2017-03-10T12:19:55Z

Because you were interested in the open issue, can you have a look to this one, @lbergelson? It still needs #810 for simplify some code paths, but I think that this could be cool for others here. I'm already using this code in my own tools, and it works amazingly!

codecov-io · 2017-03-10T12:28:23Z

Codecov Report

Merging #822 into master will decrease coverage by 0.096%.
The diff coverage is 49.565%.

@@               Coverage Diff               @@
##              master      #822       +/-   ##
===============================================
- Coverage     66.198%   66.101%   -0.096%     
+ Complexity      7640      7631        -9     
===============================================
  Files            535       539        +4     
  Lines          32406     32491       +85     
  Branches        5514      5514               
===============================================
+ Hits           21452     21477       +25     
- Misses          8790      8847       +57     
- Partials        2164      2167        +3

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/htsjdk/tribble/writer/FeatureEncoder.java	`0% <0%> (ø)`	`0 <0> (?)`
...va/htsjdk/tribble/writer/FeatureWriterFactory.java	`0% <0%> (ø)`	`0 <0> (?)`
.../java/htsjdk/tribble/writer/FeatureWriterImpl.java	`100% <100%> (ø)`	`6 <6> (?)`
...java/htsjdk/tribble/writer/AsyncFeatureWriter.java	`80% <80%> (ø)`	`7 <7> (?)`
...main/java/htsjdk/tribble/bed/BEDSimpleEncoder.java	`85.714% <85.714%> (ø)`	`3 <3> (?)`
...a/htsjdk/tribble/writer/IndexingFeatureWriter.java	`87.5% <87.5%> (ø)`	`5 <5> (?)`
.../samtools/seekablestream/SeekableMemoryStream.java	`95% <0%> (-5%)`	`11% <0%> (-1%)`
.../htsjdk/tribble/index/tabix/TabixIndexCreator.java	`84.848% <0%> (-4.545%)`	`14% <0%> (-1%)`
...dk/samtools/seekablestream/SeekableHTTPStream.java	`54.545% <0%> (-3.03%)`	`9% <0%> (-4%)`
...ntcontext/writer/IndexingVariantContextWriter.java	`83.333% <0%> (-2.381%)`	`15% <0%> (-1%)`
... and 25 more

lbergelson · 2017-03-13T22:26:57Z

@magicDGS Sorry I haven't gotten back to you on this. I think we're very interested in this but I haven't had time to take a proper look. It's a big update and there's an upcoming deadline for some of my own work that's taking priority.

magicDGS · 2017-03-14T04:49:29Z

No worries @lbergelson. I'm already using this code in another project, but I though that it would be nice to have support for this in HTSJDK. Whenever you have time I'm up for working in your suggestions...

magicDGS · 2017-05-08T15:13:42Z

Friendly ping here @lbergelson!

droazen · 2018-01-25T20:23:16Z

@jonn-smith has volunteered to review this old branch, as he is finding the lack of tribble writing support quite awkward in his current project.

jonn-smith · 2018-01-25T20:26:22Z

@magicDGS Can you rebase this and I'll start taking a look?

magicDGS · 2018-01-25T21:08:10Z

@jonn-smith - I rebased, and I hope that it works with the current implementation of bgzip, which changed a lot after I had the last look to this branch.

Thanks for looking into this, will be really, really helpful!

yfarjoun · 2017-09-26T13:43:07Z

src/main/java/htsjdk/tribble/writer/FeatureWriterFactory.java

+ *
+ * @author Daniel Gomez-Sanchez (magicDGS)
+ */
+public final class FeatureWriterFactory {


this class is completely untested....

I was working slowly on the tests until someone takes a look to the design, because I didn't want to invest time on something that might change completely the design after review. This class will have tests after the rest of components are properly tested and reviewed.

yfarjoun · 2017-09-26T13:44:13Z

src/main/java/htsjdk/tribble/writer/IndexingFeatureWriter.java

+ *
+ * @author Daniel Gomez-Sanchez (magicDGS)
+ */
+final class IndexingFeatureWriter<F extends Feature> extends FeatureWriterImpl<F> {


tests not running?

In codecov it appears with 87.5% coverage (only sequence dictionary added to the index is uncovered)

I checked it here: https://codecov.io/gh/samtools/htsjdk/compare/88edbf3e906666f22aa1e759d91208296ab503f9...852c84273543579d9d20225ceb354c254c46144c/diff#diff-c3JjL21haW4vamF2YS9odHNqZGsvdHJpYmJsZS93cml0ZXIvSW5kZXhpbmdGZWF0dXJlV3JpdGVyLmphdmE=

I'll rebase to check if it is a problem of the diff

yfarjoun · 2017-09-26T13:45:36Z

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

+
+    /**
+     * Writes as a BED feature. Note: BED coordinates are 0-based, so the start position returned by
+     * {@link Feature#getStart()} may be different thant the output start position.


thant->than

- Encoder-based implementation - Implementation for indexing on the fly

yfarjoun · 2019-02-04T19:24:15Z

@jonn-smith are you going to get around to reviewing this?

yfarjoun · 2019-03-25T18:55:59Z

Another friendly ping @jonn-smith. This was rebased as you asked a year ago....

yfarjoun · 2019-07-29T18:47:14Z

src/main/java/htsjdk/tribble/writer/FeatureWriterImpl.java

+     * function the line buffer is reset so the contents of the buffer can be reused
+     */
+    private void writeAndResetBuffer() throws IOException {
+        writer.flush();


do we really want to flush on every record? seems wasteful

yfarjoun · 2019-07-29T18:49:42Z

src/main/java/htsjdk/tribble/writer/FeatureWriterImpl.java

+ *
+ * @author Daniel Gomez-Sanchez (magicDGS)
+ */
+class FeatureWriterImpl<F extends Feature> implements FeatureWriter<F> {


rename LineFeatureWriter

yfarjoun · 2019-07-29T18:53:21Z

src/main/java/htsjdk/tribble/writer/IndexingFeatureWriter.java

+    }
+
+    /** Wraps if necessary the output stream. */
+    private static OutputStream asLocationAwareStream(final OutputStream outputStream) {


why not return OutputStream

yfarjoun · 2019-07-29T18:54:57Z

src/main/java/htsjdk/tribble/writer/IndexingFeatureWriter.java

+        this.indexPath = indexPath;
+        this.refDict = refDict;
+        // this is already a LocationAware
+        this.location = (LocationAware) getOutputStream();


perhaps override getOutputStream() and do the cast there.

yfarjoun · 2019-07-29T18:59:14Z

src/main/java/htsjdk/tribble/writer/IndexingFeatureWriter.java

+    @Override
+    public void close() throws IOException {
+        if (refDict != null) {
+            indexer.setIndexSequenceDictionary(refDict);


any reason for this to be here rather than the constructor?

yfarjoun · 2019-08-12T18:21:56Z

src/main/java/htsjdk/tribble/writer/FeatureWriter.java

+     * @throws IOException                     if an IO error occurs.
+     * @throws htsjdk.tribble.TribbleException if an encoding error occurs.
+     */
+    public void writeHeader(final Object header) throws IOException;


Should be H type rather than Object

yfarjoun · 2019-08-12T18:37:36Z

src/main/java/htsjdk/tribble/writer/FeatureWriterFactory.java

+public final class FeatureWriterFactory {
+
+    /** Default approach for create tribble indexes. */
+    public static final IndexFactory.IndexBalanceApproach DEFAULT_INDEX_BALANCE_APPROACH = IndexFactory.IndexBalanceApproach.FOR_SEEK_TIME;


woudl be ince to have a way to inject implementation-specific options.

for example: compression level for gzip-compressed files. indexing options, specific version of writer if applicable.

yfarjoun · 2019-08-12T18:40:16Z

src/main/java/htsjdk/tribble/writer/FeatureWriterFactory.java

+
+            final IndexCreator idxCreator;
+            final Path indexPath;
+            if (blockCompressed) {


would it make sense to have a factory for the index itself?

magicDGS force-pushed the dgs_tribble_writing_support branch from e6db510 to c2721e2 Compare March 10, 2017 16:37

magicDGS mentioned this pull request Mar 14, 2017

core function of htsjdk.variant.vcf.VCFEncoder using java.lang.Appendable instead of StringBuilder #824

Closed

5 tasks

magicDGS force-pushed the dgs_tribble_writing_support branch from c2721e2 to 9cd4459 Compare June 26, 2017 16:22

magicDGS mentioned this pull request Jun 26, 2017

implement bed file writer broadinstitute/gatk#1201

Open

magicDGS force-pushed the dgs_tribble_writing_support branch from 9cd4459 to 88ac45b Compare June 27, 2017 12:54

This was referenced Jul 18, 2017

Add MAF write support to htsjdk broadinstitute/gatk#3280

Closed

VCFHeader, VCFCodec and VCFHeaderLine refactoring for VCF4.3/BCF2.2 #835

Closed

magicDGS force-pushed the dgs_tribble_writing_support branch from 27f9483 to 8487f19 Compare August 25, 2017 15:53

droazen self-requested a review October 25, 2017 16:23

droazen self-assigned this Oct 25, 2017

magicDGS force-pushed the dgs_tribble_writing_support branch from 7b106b9 to 3277b79 Compare January 25, 2018 21:08

magicDGS mentioned this pull request Feb 6, 2018

Publish the roadmap document for HTSJDK 3.0 broadinstitute/gatk#4340

Open

yfarjoun reviewed Mar 20, 2018

View reviewed changes

magicDGS added 9 commits March 21, 2018 11:04

Basic interfaces for writing and encoding

12a7a35

BEDSimpleEncoder as an example of an encoder implementation

e5ff4ef

FeatureWriter implementations

c427094

- Encoder-based implementation - Implementation for indexing on the fly

AsyncFeature writer implementation

13a7e19

Add FeatureWriterFactory for building writers

66fd3d6

Complete path support with samtools#810

4c67b5e

Add HtsjdkTest marker

9eb5c2a

Add simple tests for FeatureWriterImpl

9994e97

Extract FeatureWriterTester

99d025a

magicDGS added 5 commits March 21, 2018 11:04

Add AsyncFeatureWriterTest

e71eb7f

Add test for compatibility with BGZIP

4473501

Add tests for IndexingFeatureWriter

7d5790c

Self-notes for FeatureWriterFactory

0dd1d48

Fix typo

bc7e6e6

magicDGS force-pushed the dgs_tribble_writing_support branch from 852c842 to bc7e6e6 Compare March 21, 2018 10:05

magicDGS mentioned this pull request Apr 20, 2018

HTSJDK requires defined processes and governance #871

Open

yfarjoun assigned lbergelson and unassigned droazen May 1, 2018

lbergelson removed their assignment May 1, 2018

magicDGS mentioned this pull request Jun 1, 2018

Improve output format for ProperStatWindowEngine magicDGS/ReadTools#459

Open

magicDGS mentioned this pull request Jun 11, 2018

Make public SamReader.Type methods to allow other implementations #1144

Merged

5 tasks

magicDGS mentioned this pull request Oct 27, 2018

Design codec interface samtools/htsjdk-next-beta#30

Open

jonn-smith self-requested a review February 4, 2019 19:32

jonn-smith self-assigned this Feb 4, 2019

yfarjoun reviewed Jul 29, 2019

View reviewed changes

yfarjoun reviewed Aug 12, 2019

View reviewed changes

yfarjoun added the Large 🐋 label Sep 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tribble writing support #822

Tribble writing support #822

magicDGS commented Mar 10, 2017 •

edited

Loading

magicDGS commented Mar 10, 2017

codecov-io commented Mar 10, 2017 •

edited

Loading

lbergelson commented Mar 13, 2017

magicDGS commented Mar 14, 2017

magicDGS commented May 8, 2017

droazen commented Jan 25, 2018

jonn-smith commented Jan 25, 2018

magicDGS commented Jan 25, 2018

yfarjoun Sep 26, 2017

magicDGS Mar 21, 2018

yfarjoun Sep 26, 2017

magicDGS Mar 21, 2018

magicDGS Mar 21, 2018

yfarjoun Sep 26, 2017

magicDGS Mar 21, 2018

yfarjoun commented Feb 4, 2019

yfarjoun commented Mar 25, 2019

yfarjoun Jul 29, 2019

yfarjoun Jul 29, 2019

yfarjoun Jul 29, 2019

yfarjoun Jul 29, 2019

yfarjoun Jul 29, 2019

yfarjoun Aug 12, 2019

yfarjoun Aug 12, 2019

yfarjoun Aug 12, 2019

Tribble writing support #822

Are you sure you want to change the base?

Tribble writing support #822

Conversation

magicDGS commented Mar 10, 2017 • edited Loading

Description

Checklist

magicDGS commented Mar 10, 2017

codecov-io commented Mar 10, 2017 • edited Loading

Codecov Report

lbergelson commented Mar 13, 2017

magicDGS commented Mar 14, 2017

magicDGS commented May 8, 2017

droazen commented Jan 25, 2018

jonn-smith commented Jan 25, 2018

magicDGS commented Jan 25, 2018

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 Feb 4, 2019

yfarjoun commented Mar 25, 2019

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 Mar 10, 2017 •

edited

Loading

codecov-io commented Mar 10, 2017 •

edited

Loading