update CramCompressionRecord.isPlaced() to make it APDelta-aware

[jmthibault79]

Description

isPlaced() was not checking for the APDelta flag, thus inappropriately marking some records as unplaced.

Error introduced on 1/30 with #1266

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)

[codecov-io]

Codecov Report

Merging #1284 into master will increase coverage by 0.278%.
The diff coverage is 97.222%.

@@              Coverage Diff               @@
##             master     #1284       +/-   ##
==============================================
+ Coverage     67.49%   67.768%   +0.278%     
- Complexity     8150      8345      +195     
==============================================
  Files           558       561        +3     
  Lines         33365     33929      +564     
  Branches       5608      5752      +144     
==============================================
+ Hits          22518     22993      +475     
- Misses         8658      8733       +75     
- Partials       2189      2203       +14

Impacted Files	Coverage Δ	Complexity Δ
...ava/htsjdk/samtools/cram/build/CramNormalizer.java	81.25% <100%> (+0.107%)	57 <0> (+1)	⬆️
...ain/java/htsjdk/samtools/cram/structure/Slice.java	63.008% <100%> (+2.76%)	76 <1> (+38)	⬆️
...samtools/cram/structure/CramCompressionRecord.java	82.308% <100%> (+3.141%)	96 <21> (+8)	⬆️
...ava/htsjdk/samtools/CRAMContainerStreamWriter.java	70.612% <50%> (-0.289%)	54 <0> (ø)
...c/main/java/htsjdk/samtools/util/SnappyLoader.java	78.571% <0%> (-2.198%)	9% <0%> (ø)
src/main/java/htsjdk/samtools/SamReader.java	81.818% <0%> (-0.94%)	0% <0%> (ø)
...va/htsjdk/samtools/SAMSequenceDictionaryCodec.java	86.957% <0%> (-0.543%)	8% <0%> (+2%)
src/main/java/htsjdk/samtools/util/CigarUtil.java	30.303% <0%> (-0.466%)	14% <0%> (ø)
...java/htsjdk/samtools/cram/ref/ReferenceSource.java	46.364% <0%> (-0.425%)	19% <0%> (ø)
...htsjdk/tribble/readers/LongLineBufferedReader.java	29.944% <0%> (-0.342%)	15% <0%> (ø)
... and 32 more

[cmnbroad]

A few comments/questions.

[cmnbroad]

This one seems inverted - it will be false for coordinate sorted. Since no tests failed, we should add one somewhere that verifies this is working correctly.

[jmthibault79]

oops, good catch.

Unfortunately, I believe that it would take me a substantial amount of time to make sense of CramNormalizer and write a reasonable test for it, so I don't think that's feasible with the quick turnaround time we want for this PR.

[cmnbroad]

Is there any test in this PR that would have failed before without the fix for APDelta, but passes now so at least we can be sure that we've fixed it ? I assume we didn't have one before this bug was introduced, since no test caught the issue ? Without that, I'm not even sure I'd recognize the failure if I saw it. Ideally we'd create a ticket describing it (that will be fixed by this PR) so we have a record in case anyone sees it using the interim version that had the issue.

[jmthibault79]

Yes. The pair of SliceTests cases starting with the comment on line 97 calls this out explicitly.

On a lower level, the combinations in CramCompressionRecordTest.placedTests show that the APDelta flag does affect results, and some would fail without this fix.

[cmnbroad]

I think its worth investing in some test cleanup for this one - lots of test comments.

[lbergelson]

It would be nicer if CramCompressionRecord knew if it was delta encoded, but this is fine too.