Update CRAM conversion and round trip tests. #720

Merged
merged 1 commit into from Feb 8, 2017

Conversation

Projects
None yet
5 participants
Contributor

cmnbroad commented Oct 7, 2016 edited by yfarjoun

The existing CRAM compliance tests do only partial record validation after conversion, rather than testing for full record equality. This PR separates out the tests that fail if full record validation is used (tickets filed) and:
-changes the remaining tests to require full SAMRecord equality.
-Adds a BAM->CRAM->BAM round trip validation test based on an NA12878 subset.
-Removes a couple of unreferenced or duplicate alignment and reference files.

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)

Coverage Status

Coverage increased (+0.1%) to 69.151% when pulling 78cc3a2 on cmnbroad:cn_cram_roundtrip into 88b6719 on samtools:master.

Coverage Status

Coverage increased (+0.1%) to 69.151% when pulling e5304a6 on cmnbroad:cn_cram_roundtrip into 88b6719 on samtools:master.

yfarjoun added the cram label Nov 22, 2016

droazen referenced this pull request Nov 29, 2016

Open

Review "Party" #548

@vadimzalunin

except for 2 minor changes looks very good in general, binary BAM comparison is my only concern as this depends on system properties.

+ roundTrippedBAMFile.deleteOnExit();
+ writeRecordsToFile(cramRecords, roundTrippedBAMFile, referenceFile, samHeader);
+
+ Assert.assertTrue(compareMD5Digests(roundTrippedBAMFile, originalBAMInputFile));
@vadimzalunin

vadimzalunin Jan 24, 2017

Contributor

not sure about binary equality of BAM files, for example changing compression level will break this.

+ try (FileInputStream rtStream = new FileInputStream(tempRoundTrippedBAMFile);
+ DigestInputStream rtDigestStream = new DigestInputStream(rtStream, rtMD);
+ FileInputStream origStream = new FileInputStream(tempOriginalBAMFile);
+ DigestInputStream origDigestStream = new DigestInputStream(origStream, origMD);) {
@vadimzalunin

vadimzalunin Jan 24, 2017

Contributor

remove semicolumn

+ final List<SAMRecord> recs,
+ final File targetFile,
+ final File referenceFile,
+ final SAMFileHeader samHeader) throws IOException {
@vadimzalunin

vadimzalunin Jan 24, 2017

Contributor

remove 'throws IOException'

Contributor

vadimzalunin commented Jan 30, 2017

summoning @droazen @yfarjoun @cmnbroad into this thread
It appears I'm blocked on this PR... Given the changes I've requested are not important, can we just merge this?

Contributor

cmnbroad commented Jan 30, 2017 edited

Its fine with me - this PR only has test changes.

@cmnbroad cmnbroad Add/improve CRAM roundtripping tests.
b54150e

codecov-io commented Feb 7, 2017 edited

Codecov Report

Merging #720 into master will increase coverage by 0.114%.

@@               Coverage Diff               @@
##              master      #720       +/-   ##
===============================================
+ Coverage     64.623%   64.737%   +0.114%     
- Complexity      7113      7137       +24     
===============================================
  Files            525       525               
  Lines          31679     31679               
  Branches        5414      5414               
===============================================
+ Hits           20472     20508       +36     
+ Misses          9061      9029       -32     
+ Partials        2146      2142        -4
Impacted Files Coverage Δ Complexity Δ
...va/htsjdk/samtools/util/AsyncBufferedIterator.java 68.478% <ø> (-1.087%) 17% <ø> (ø)
src/main/java/htsjdk/samtools/SAMRecord.java 65.931% <ø> (+0.123%) 285% <ø> (+2%)
...ava/htsjdk/samtools/cram/build/CramNormalizer.java 81.356% <ø> (+0.565%) 57% <ø> (+1%)
...main/java/htsjdk/samtools/SAMRecordSetBuilder.java 89.113% <ø> (+0.806%) 64% <ø> (+2%)
...samtools/cram/structure/CramCompressionRecord.java 69.748% <ø> (+0.84%) 77% <ø> (+1%)
...dk/samtools/seekablestream/SeekableHTTPStream.java 56.061% <ø> (+1.515%) 10% <ø> (+1%)
src/main/java/htsjdk/samtools/BinaryTagCodec.java 76.243% <ø> (+17.127%) 68% <ø> (+17%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b05d1f2...b54150e. Read the comment docs.

Contributor

cmnbroad commented Feb 8, 2017

I'm not sure why the codecov report shows a change in src/main/java/htsjdk/samtools/util/AsyncBufferedIterator.java that looks unrelated to this PR, but is looks to be timing related. The tests are passing, so I"m merging.

@cmnbroad cmnbroad merged commit 7ef0a84 into samtools:master Feb 8, 2017

3 of 4 checks passed

codecov/changes 1 file has unexpected coverage changes not visible in diff.
Details
codecov/patch Coverage not affected when comparing b05d1f2...b54150e
Details
codecov/project 64.737% (+0.114%) compared to b05d1f2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

cmnbroad deleted the cmnbroad:cn_cram_roundtrip branch Feb 10, 2017

@vadimzalunin vadimzalunin added a commit to vadimzalunin/htsjdk that referenced this pull request Mar 14, 2017

@cmnbroad @vadimzalunin cmnbroad + vadimzalunin Add/improve CRAM roundtripping tests. (#720) 42146d7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment