Only compute BIN on BAM write and on index building #1258

tfenne · 2019-01-10T15:52:08Z

Description

Prior to this change if you had a SAM or BAM file with records mapped to references > 512mb, SAMRecord would log a warning per record mapped to those contigs!! It would also blindly compute an invalid BIN value and store it.

This change removes the stored indexingBin field in SAMRecord and has it computed only when needed - i.e. when writing a BAM file in BAMRecordCodec and during index building. In BAMRecordCoded it will warn once when the first read mapped to a long contig is found, and will write a bin value of 0, the same as we do for unmapped reads. When index building it will throw an exception since BAI cannot support reads mapped beyond 512mb.

I ran into a lot of issues with how this interacted with the long cigar code. @yfarjoun I ended up resolving those by modifying the code in BAMRecordCodec slightly. Instead of calling setCigar() with the sentinal cigar, it creates a local variable called cigarToWrite and assigns either the real cigar or the sentinal cigar as appropriate. I also added a line at the end of the encode() to remove the CG tag from the record post-encoding, so that a pass through BAMRecordCodec now leaves the record largely untouched.

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)

…M and during index creation.

…in BAMRecordCodec.

codecov-io · 2019-01-10T16:04:46Z

Codecov Report

Merging #1258 into master will increase coverage by 0.051%.
The diff coverage is 70.968%.

@@               Coverage Diff               @@
##              master     #1258       +/-   ##
===============================================
+ Coverage     69.366%   69.417%   +0.051%     
- Complexity      8157      8201       +44     
===============================================
  Files            548       548               
  Lines          32706     32842      +136     
  Branches        5523      5556       +33     
===============================================
+ Hits           22687     22798      +111     
- Misses          7799      7822       +23     
- Partials        2220      2222        +2

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/htsjdk/samtools/BAMRecord.java	`83.14% <ø> (+1.637%)`	`53 <0> (+2)`	⬆️
src/main/java/htsjdk/samtools/SAMUtils.java	`59.5% <0%> (-0.149%)`	`127 <0> (ø)`
src/main/java/htsjdk/samtools/BAMIndexer.java	`76.531% <100%> (-0.237%)`	`17 <0> (ø)`
.../main/java/htsjdk/samtools/SAMValidationError.java	`96.117% <100%> (+0.038%)`	`9 <0> (ø)`	⬇️
...rc/main/java/htsjdk/samtools/SamFileValidator.java	`84.211% <20%> (-0.743%)`	`82 <1> (+1)`
src/main/java/htsjdk/samtools/SAMRecord.java	`67.271% <40%> (-0.524%)`	`300 <2> (-15)`
src/main/java/htsjdk/samtools/BAMRecordCodec.java	`79.839% <94.118%> (+0.847%)`	`25 <0> (+4)`	⬆️
src/main/java/htsjdk/samtools/cram/CRAIIndex.java	`89.362% <0%> (+1.126%)`	`43% <0%> (+15%)`	⬆️
...java/htsjdk/samtools/cram/structure/Container.java	`83.871% <0%> (+2.621%)`	`14% <0%> (+7%)`	⬆️
... and 3 more

yfarjoun

Thanks for this. in particular, given the long-cigar shinanigans, I like not over-using setCigar and getCigar in the encode method....

src/main/java/htsjdk/samtools/SAMRecord.java

src/main/java/htsjdk/samtools/SAMUtils.java

src/test/java/htsjdk/samtools/BAMFileWriterTest.java

yfarjoun · 2019-01-10T23:41:26Z

src/test/java/htsjdk/samtools/CRAMComplianceTest.java

@@ -294,9 +293,6 @@ public void testBAMThroughCRAMRoundTrip() throws IOException, NoSuchAlgorithmExc
        // retrieve all records from the bam and reset the indexing bins to keep comparisons with


update the comment?

I've just removed the comment as all that line now does is load the records from a file and that didn't seem comment-worthy.

yfarjoun · 2019-01-10T23:41:33Z

src/test/java/htsjdk/samtools/CRAMComplianceTest.java

@@ -327,9 +323,6 @@ public void testBAMThroughCRAMRoundTripViaPath() throws IOException, NoSuchAlgor
        // retrieve all records from the bam and reset the indexing bins to keep comparisons with


update comment

src/main/java/htsjdk/samtools/SAMRecord.java

yfarjoun · 2019-01-10T23:46:46Z

src/main/java/htsjdk/samtools/SAMRecord.java

-    void setIndexingBin(final Integer mIndexingBin) {
-        this.mIndexingBin = mIndexingBin;
-    }
-
    /**
     * Does not change state of this.


update comment to reflect the fact that there is a possible side-effect due to the need to parse the cigar.

yfarjoun · 2019-01-11T20:42:41Z

@lbergelson case to cast a glance?

tfenne · 2019-01-12T00:34:39Z

@lbergelson I'd be very grateful if you could let me know, in the next few days, whether you'd like to review this PR or if you're happy enough with @yfarjoun's review. Thanks!

lbergelson · 2019-01-14T14:25:53Z

@tfenne Sorry! I will review.

lbergelson

@tfenne This seems pretty reasonable to me. A few minor comments.

lbergelson · 2019-01-14T14:34:29Z

src/main/java/htsjdk/samtools/BAMRecordCodec.java

@@ -108,18 +108,20 @@ public void encode(final SAMRecord alignment) {
        final int readLength = alignment.getReadLength();

        // if cigar is too long, put into CG tag and replace with sentinel value
-        if (alignment.getCigarLength() > BAMRecord.MAX_CIGAR_OPERATORS) {
+        final Cigar cigarToWrite;
+        final boolean cigarSwitcharoo = alignment.getCigar().numCigarElements() > BAMRecord.MAX_CIGAR_OPERATORS;


Is the change from getCigarLength -> getCigar().numCigarElements() for a reason? If not I might keep getCigarLength. If it is then it needs a comment because it's not obvious to me.

The short answer is that some of the tests fail if I leave it as getCigarLength() and they all pass if I just call getCigar().numCigarElements(). I'm honestly not 100% sure why and that code has gotten horribly complicated with all the long cigar support. I'll add a comment.

That scares the hell out of me... those seem like they should be identical..

lbergelson · 2019-01-14T14:45:18Z

src/main/java/htsjdk/samtools/SAMRecord.java

-                    if (getReferenceName() != null) {
-                        LOG.warn(String.format("%s reference length is too large for BAM bin field. %s record bin field value is incorrect.",
-                                getReferenceName(), getReadName()));
-                    } else {


I think keeping the reference length is too long for bam warning is a good idea. It's still problem you would want to know about if you're validating your bam I think.

This is actually my primary reason for the PR in the first place! With this check here, unless you set validation stringency to silent, you get a warning logged per record when reading. So I'm getting millions of warnings!
My take is that it's perfectly valid SAM/BAM to have records mapped to references that are longer than BAI can handle, but it's problematic when you try to index with BAI. That's why I removed the warnings, and made sure it was a clear failure when building a BAI. If I leave this in, then anyone using either unindexed BAMs or CSI-indexed BAMs will see millions of unnecessary warnings.

Oh.. of course.. that makes sense. Maybe there's a way we can have ValidateSam warn exactly once if it encounters reads like that but not have it happen in the general case of just processing a sam file with validation on.

lbergelson · 2019-01-14T14:46:20Z

src/main/java/htsjdk/samtools/SAMUtils.java

+     * @deprecated because the method does the exact opposite of what it says.  Use the correctly named
+     *             isReferenceSequenceIncompatibleWithBAI() instead.
+     */
+    @Deprecated public static boolean isReferenceSequenceCompatibleWithBAI(final SAMSequenceRecord sequence) {


oh god, that's horrible

Indeed. I was really confused initially.

…s().

pshapiro4broad · 2019-01-15T20:50:54Z

src/main/java/htsjdk/samtools/BAMIndexer.java

-                    return (binNumber == null ? rec.computeIndexingBin() : binNumber);
-
-                }
+                public Integer getIndexingBin() { return rec.computeIndexingBin(); }


format over lines, e.g.

public Integer getIndexingBin() { return rec.computeIndexingBin(); }

pshapiro4broad · 2019-01-15T20:51:17Z

src/main/java/htsjdk/samtools/BAMRecordCodec.java

            alignment.setAttribute(CG.name(), cigarEncoding);
+            cigarToWrite = makeSentinelCigar(alignment.getCigar());
+        }


format else on previous line

} else {

src/main/java/htsjdk/samtools/BAMRecordCodec.java

pshapiro4broad · 2019-01-15T20:57:21Z

src/main/java/htsjdk/samtools/BAMRecordCodec.java

+        final boolean tooLarge = sequence != null && SAMUtils.isReferenceSequenceIncompatibleWithBAI(sequence);
+        if (!isReferenceSizeWarningShowed & tooLarge && rec.getValidationStringency() != ValidationStringency.SILENT) {
+            LOG.warn("Reference length is too large for BAM bin field.");
+            LOG.warn("Reads on references longer than " + GenomicIndexUtil.BIN_GENOMIC_SPAN + "bp will have bin set to 0.");


missing space in warning

Suggested change

LOG.warn("Reads on references longer than " + GenomicIndexUtil.BIN_GENOMIC_SPAN + "bp will have bin set to 0.");

LOG.warn("Reads on references longer than " + GenomicIndexUtil.BIN_GENOMIC_SPAN + " bp will have bin set to 0.");

This is intentional. It's normal to write 1234bp vs. 1234 bp.

pshapiro4broad · 2019-01-15T20:58:44Z

src/main/java/htsjdk/samtools/SAMRecord.java

-    void setIndexingBin(final Integer mIndexingBin) {
-        this.mIndexingBin = mIndexingBin;
-    }
+    @Deprecated() public int computeIndexingBinIfAbsent(final SAMRecord alignment) { return alignment.computeIndexingBin(); }


format across lines

src/main/java/htsjdk/samtools/SAMRecord.java

src/test/java/htsjdk/samtools/BAMCigarOverflowTest.java

src/test/java/htsjdk/samtools/BAMFileWriterTest.java

…der is too long for BAI indexing.

Co-Authored-By: tfenne <tfenne@tfenne.com>

lbergelson

👍 Thank you for adding the warning.

tfenne added 2 commits January 9, 2019 19:14

Remove indexingBin from SAMRecord and only compute it when writing BA…

6e3d454

…M and during index creation.

Fix up the long cigar tests by not modifying the cigar on the record …

0c6ab4e

…in BAMRecordCodec.

tfenne self-assigned this Jan 10, 2019

tfenne requested review from yfarjoun and lbergelson January 10, 2019 15:52

yfarjoun requested changes Jan 10, 2019

View reviewed changes

Addressing yfarjoun's review comments.

ef54a3c

yfarjoun approved these changes Jan 11, 2019

View reviewed changes

lbergelson reviewed Jan 14, 2019

View reviewed changes

Added comment about .getCigarLength() vs. .getCigar().numCigarElement…

336f4d2

…s().

pshapiro4broad reviewed Jan 15, 2019

View reviewed changes

tfenne and others added 2 commits January 16, 2019 08:58

Added a single warning in SamFileValidator if any sequence in the hea…

2f49d75

…der is too long for BAI indexing.

Apply suggestions from code review

31f093a

Co-Authored-By: tfenne <tfenne@tfenne.com>

lbergelson approved these changes Jan 16, 2019

View reviewed changes

tfenne merged commit 15ec7da into master Jan 16, 2019

tfenne deleted the tf_maybe_dont_compute_bin branch January 16, 2019 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only compute BIN on BAM write and on index building #1258

Only compute BIN on BAM write and on index building #1258

tfenne commented Jan 10, 2019

codecov-io commented Jan 10, 2019 •

edited

Loading

yfarjoun left a comment

yfarjoun Jan 10, 2019

tfenne Jan 11, 2019

yfarjoun Jan 10, 2019

tfenne Jan 11, 2019

yfarjoun Jan 10, 2019

tfenne Jan 11, 2019

yfarjoun commented Jan 11, 2019

tfenne commented Jan 12, 2019

lbergelson commented Jan 14, 2019

lbergelson left a comment

lbergelson Jan 14, 2019

tfenne Jan 15, 2019

lbergelson Jan 15, 2019

lbergelson Jan 14, 2019

tfenne Jan 15, 2019

lbergelson Jan 15, 2019

lbergelson Jan 14, 2019

tfenne Jan 15, 2019

pshapiro4broad Jan 15, 2019

pshapiro4broad Jan 15, 2019

pshapiro4broad Jan 15, 2019

tfenne Jan 16, 2019

pshapiro4broad Jan 15, 2019

lbergelson left a comment

		@@ -294,9 +293,6 @@ public void testBAMThroughCRAMRoundTrip() throws IOException, NoSuchAlgorithmExc
		// retrieve all records from the bam and reset the indexing bins to keep comparisons with

		@@ -327,9 +323,6 @@ public void testBAMThroughCRAMRoundTripViaPath() throws IOException, NoSuchAlgor
		// retrieve all records from the bam and reset the indexing bins to keep comparisons with

	LOG.warn("Reads on references longer than " + GenomicIndexUtil.BIN_GENOMIC_SPAN + "bp will have bin set to 0.");
	LOG.warn("Reads on references longer than " + GenomicIndexUtil.BIN_GENOMIC_SPAN + " bp will have bin set to 0.");

Only compute BIN on BAM write and on index building #1258

Only compute BIN on BAM write and on index building #1258

Conversation

tfenne commented Jan 10, 2019

Description

Checklist

codecov-io commented Jan 10, 2019 • edited Loading

Codecov Report

yfarjoun 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

yfarjoun commented Jan 11, 2019

tfenne commented Jan 12, 2019

lbergelson commented Jan 14, 2019

lbergelson 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

lbergelson left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 10, 2019 •

edited

Loading