Wire up CRAM 3.1 codecs for reading. #1736

cmnbroad · 2025-01-22T14:53:51Z

A few notes:

The carrot tests (.carrot folder and CarrotTest.java) are still a WIP.
The (beta API) FASTACodecV1_0 changes are to fix an issue that came up when I added the (beta api) CRAM 3.1 codec and a corresponding test; the codec did not properly handle gzipped fasta files.
The NameTokenisationEncoder and EncodeToken class changes are to fix an issue discovered on the separate CRAM 3.1 writer branch.
The CRAMContextModel class (unused for now) is an accumulator class that is threaded throughout the CRAM write code for use by write profiles that depend on having access to context.

lbergelson

@cmnbroad Some comments. One potential bug and a request to rename the new build commands so it's clear they're for a special test build.

lbergelson · 2025-04-02T19:19:21Z

Dockerfile

@@ -0,0 +1,13 @@
+# This docker build is here to enable Carrot tests


I don't know if there's a way to have carrot build a non-default docker image, but if that's possible I would call this something like CarrotDockerFile so people don't naively try to use it for non-carrot things.

Commit removed, to be revived on the write branch.

lbergelson · 2025-04-02T19:19:56Z

build.gradle

@@ -79,6 +79,21 @@ jar {
    }
 }

+// A shadow jar that includes all dependencies PLUS test code, for use with carrot tests
+shadowJar {


Lets make separate carrotShadowJar task instead of using the default. Again, people might just run shadowjar and assume it's the standard htsjdk thing.

lbergelson · 2025-04-02T19:23:57Z

src/main/java/htsjdk/beta/plugin/registry/ReadsResolver.java

+     * the newest version, since it has no write implementation yet.
+     */
+    @Override
+    protected List<ReadsCodec> filterByVersion(final List<ReadsCodec> candidateCodecs, final HtsVersion htsVersion) {


Not for this PR, but I wonder if we should make this something that exists generically.

lbergelson · 2025-04-02T19:34:54Z

src/main/java/htsjdk/samtools/cram/compression/CompressionUtils.java

+            return buffer.array();
+        }
+
+        final byte[] bytes = new byte[buffer.remaining()];


I probably am misunderstanding, but doesn't remaining return the length between the current position and the end? Isn't what you want the length from arrayOffset() to limit()?

Oh yeah, very good catch. Fixed.

lbergelson · 2025-04-02T19:55:10Z

src/main/java/htsjdk/samtools/cram/structure/CompressorCache.java

@@ -57,6 +64,9 @@ public ExternalCompressor getCompressorForMethod(
            final int compressorSpecificArg) {
        switch (compressionMethod) {


This could be a switch expression which lets you drop the default condition. It's fine though.

lbergelson · 2025-04-02T19:56:40Z

src/main/java/htsjdk/samtools/cram/structure/CompressorCache.java

+                        sharedRANSNx16Decode = new RANSNx16Decode();
+                    }
+                    compressorCache.put(
+                            new Tuple(BlockCompressionMethod.RANSNx16, ransArg),


Nitpick: Tuple<>

This might be the sort of place where a private CompressorConfig record type could clarify things instead of the awkward tuples.

lbergelson · 2025-04-02T20:02:11Z

src/main/java/htsjdk/samtools/cram/structure/Slice.java

@@ -56,6 +55,8 @@
 * on the corresponding reference contig.
 */
 public class Slice {
+    private final CRAMVersion cramVersion;


Ooh, can we have crams that vary version by slice?!

Let us hope not. Its just for convenience - there have always been cases where slice needs to discriminate by version when reading a slice, but for some reason I thought I needed it when writing now too - although so far there are no references to it.

lbergelson · 2025-04-08T18:45:44Z

src/test/java/htsjdk/samtools/cram/CarrotTest.java

+    //      reads file 1 (bam or cram)
+    //      reads file 2 (bam or cram)
+    //      reference
+    public static void main(final String[] args) {


This feels awkwardly placed but doing it differently seems like more of a hassle. I think you've come up with a pretty good solution.

I'm actually reverting the carrot changes in this branch (reverting the one commit) since its not complete/working anyway. I'll revive it for the write branch though.

lbergelson

@cmnbroad Some comments. One potential bug and a request to rename the new build commands so it's clear they're for a special test build.

jmarshall · 2025-04-14T22:46:27Z

The release notes for the recent 4.2.0 release note that it includes CRAM 3.1 codecs due to #1714. However it appears that, without this PR's addition to CramVersions.java in particular, the recent release remains unable to read CRAM v3.1 files.

lbergelson · 2025-04-15T00:17:18Z

@jmarshall Ah, sorry for the confusion. It's literally just the codecs in that PR. The wiring isn't in yet. It's coming soon.

jmarshall · 2025-04-15T00:24:52Z

No confusion 😄 I just wanted to make sure that this PR hadn't been inadvertently omitted from the release or anything similar. Particularly in the light of samtools/htslib#1900 and the related release notes for the (imminent?) upcoming htslib/samtools releases — cf samtools/htslib#1898 first paragraph of the addition.

This reverts commit c8ba753.

lbergelson

This looks good to me. I'm merging!

cmnbroad added the cram label Jan 22, 2025

cmnbroad force-pushed the cn_cram_3_1_wiring branch from 773b235 to c6c7ce5 Compare January 28, 2025 21:21

cmnbroad marked this pull request as ready for review February 3, 2025 13:52

cmnbroad marked this pull request as draft February 3, 2025 13:52

cmnbroad force-pushed the cn_cram_3_1_wiring branch 2 times, most recently from fd4e834 to 68c3b8a Compare February 4, 2025 13:55

cmnbroad mentioned this pull request Feb 11, 2025

CRAM 3.1 read support broadinstitute/gatk#9095

Draft

cmnbroad force-pushed the cn_cram_3_1_wiring branch 2 times, most recently from f012878 to d89abba Compare February 11, 2025 21:02

cmnbroad force-pushed the cn_cram_3_1_wiring branch 3 times, most recently from 1a5dc3a to 79792f2 Compare February 24, 2025 19:48

cmnbroad force-pushed the cn_cram_3_1_wiring branch 3 times, most recently from 840fc9d to c8ba753 Compare March 24, 2025 14:59

cmnbroad marked this pull request as ready for review March 24, 2025 18:44

cmnbroad mentioned this pull request Mar 24, 2025

Wire up CRAM 3.1 codecs for writing and a naive write profile. #1739

Open

lbergelson approved these changes Apr 8, 2025

View reviewed changes

lbergelson requested changes Apr 8, 2025

View reviewed changes

cmnbroad force-pushed the cn_cram_3_1_wiring branch from c8ba753 to 3362a10 Compare April 22, 2025 19:35

cmnbroad added 7 commits April 22, 2025 16:53

Wire up CRAM 3.1 codecs for reading.

195c326

Thread model context through CRAM write code.

6158043

Backport fixes from issues found on the CRAM 3.1 writing branch.

e6eda0e

Test/comment cleanup.

59c383e

Carrot tests - WIP.

95fe86a

Fixe CompressionUtils bug.

4f59598

Remove use of Tuple in compressor cache.

fa0eb87

Revert "Carrot tests - WIP."

144f5f8

This reverts commit c8ba753.

cmnbroad force-pushed the cn_cram_3_1_wiring branch from 3362a10 to 144f5f8 Compare April 22, 2025 20:53

lbergelson approved these changes May 9, 2025

View reviewed changes

lbergelson merged commit c6729bb into master May 9, 2025
2 of 4 checks passed

lbergelson deleted the cn_cram_3_1_wiring branch May 9, 2025 20:43

		@@ -0,0 +1,13 @@
		# This docker build is here to enable Carrot tests

		@@ -57,6 +64,9 @@ public ExternalCompressor getCompressorForMethod(
		final int compressorSpecificArg) {
		switch (compressionMethod) {

Wire up CRAM 3.1 codecs for reading. #1736

Wire up CRAM 3.1 codecs for reading. #1736

Uh oh!

Conversation

cmnbroad commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lbergelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbergelson left a comment

Choose a reason for hiding this comment

Uh oh!

jmarshall commented Apr 14, 2025

Uh oh!

lbergelson commented Apr 15, 2025

Uh oh!

jmarshall commented Apr 15, 2025

Uh oh!

lbergelson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cmnbroad commented Jan 22, 2025 •

edited

Loading