search

samtools / htsjdk

A Java API for high-throughput sequencing data (HTS) formats.
http://samtools.github.io/htsjdk/
283 stars 242 forks source link

- implementing the spec change in #1238

Closed yfarjoun closed 5 years ago

yfarjoun commented 5 years ago

https://github.com/samtools/hts-specs/pull/333

codecov-io commented 5 years ago

Codecov Report

Merging #1238 into master will increase coverage by 0.353%. The diff coverage is 100%.

@@               Coverage Diff               @@
##              master     #1238       +/-   ##
===============================================
+ Coverage     69.323%   69.676%   +0.353%     
- Complexity      8105      8321      +216     
===============================================
  Files            543       547        +4     
  Lines          32480     33069      +589     
  Branches        5501      5685      +184     
===============================================
+ Hits           22516     23041      +525     
- Misses          7755      7803       +48     
- Partials        2209      2225       +16
Impacted Files Coverage Δ Complexity Δ
...c/main/java/htsjdk/samtools/SAMSequenceRecord.java 79.167% <100%> (-0.563%) 35 <2> (-1)
...samtools/cram/compression/ExternalCompression.java 79.661% <0%> (-8.91%) 17% <0%> (+6%)
...a/htsjdk/samtools/cram/build/ContainerFactory.java 95.385% <0%> (-2.441%) 38% <0%> (+18%)
...n/java/htsjdk/samtools/cram/structure/SliceIO.java 87.209% <0%> (-1.68%) 19% <0%> (+4%)
...s/cram/structure/block/BlockCompressionMethod.java 100% <0%> (ø) 4% <0%> (?)
...amtools/cram/structure/block/BlockContentType.java 100% <0%> (ø) 4% <0%> (?)
...va/htsjdk/samtools/cram/structure/block/Block.java 75% <0%> (ø) 22% <0%> (?)
...k/samtools/cram/structure/block/ExternalBlock.java 100% <0%> (ø) 2% <0%> (?)
src/main/java/htsjdk/samtools/SAMRecord.java 67.795% <0%> (+0.117%) 315% <0%> (+1%) :arrow_up:
src/main/java/htsjdk/samtools/SAMUtils.java 59.848% <0%> (+0.505%) 126% <0%> (+1%) :arrow_up:
... and 10 more
jmarshall commented 5 years ago

I notice that this enforces RNAME compliance with the regexp regardless of any declared \@HD-VN.

Did HTSJDK consider making this check dependent on the file's VN header and allowing such RNAMEs in existing files with HD-VN≤1.5 or so? There was discussion in https://github.com/samtools/hts-specs/pull/333#issuecomment-422835741 that it seems important not to gratuitously reject existing data files (if any…) while allowing for diagnostics on newly-produced files.

yfarjoun commented 5 years ago

hmmm. this is a good point. will look into making is 1.5 and on only (not sure how....)

yfarjoun commented 5 years ago

@jmarshall, we have decided (in htsjdk) to implement a blanket rejection of non-compliant sam files. if this turns out to be an undue burden on our users, we will consider putting to work to make htsjdk more version-aware. right now we are working on the next version which should include more version-aware features and it should be easier to do such a thing there...I still think that we were right to keep the SPEC version specific and not declare all old SAM files as non-compliant.

jmarshall commented 5 years ago

Fair enough. Please take a look at the text added in that new commit on samtools/hts-specs#333 too :smile: