Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RANS Nx16 codec (Update CRAM Codecs to CRAM 3.1) #1618

Open
wants to merge 66 commits into
base: master
Choose a base branch
from

Conversation

yash-puligundla
Copy link
Contributor

@yash-puligundla yash-puligundla commented Aug 26, 2022

Description

This PR is part of an effort to upgrade CRAM to v3.1. It adds the RANS Nx16 implementation and adds changes to the existing RANS implementation to accommodate RANS Nx16.

List of Changes:

  • Add RANS Nx16 Codec
  • Add interfaces - RANSEncode, RANSDecode and RANSParams
  • Restructured existing RANS code
    • Rename the existing RANS codec to RANS4x8
    • Change the structure of the ArithmeticDecoder
    • Move the code from E04, E14, D04, and D14 into the classes where it is used
  • Add CRAMCodecCorpusTest to test RANS 4x8 and RANS Nx16 Codecs using test files from htscodecs
  • Add more test cases to RANSTest to test RANS 4x8 and RANS Nx16 Codecs

Test fails:
CRAMCodecCorpusTest fails as they require the test files from htscodecs repo.

@cmnbroad cmnbroad self-assigned this Aug 29, 2022
Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkpointing what I have so far, which only covers test code.

src/test/java/htsjdk/samtools/cram/CRAMCodecCorpus.java Outdated Show resolved Hide resolved
src/test/java/htsjdk/samtools/cram/CRAMCodecCorpus.java Outdated Show resolved Hide resolved
src/test/java/htsjdk/samtools/cram/CRAMCodecCorpus.java Outdated Show resolved Hide resolved
Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more minor comments.

src/test/java/htsjdk/samtools/cram/RANSInteropTest.java Outdated Show resolved Hide resolved
src/test/java/htsjdk/samtools/cram/RANSInteropTest.java Outdated Show resolved Hide resolved
src/test/java/htsjdk/samtools/cram/RANSInteropTest.java Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2022

Codecov Report

Merging #1618 (c63a253) into master (347c0ac) will increase coverage by 0.423%.
Report is 40 commits behind head on master.
The diff coverage is 95.548%.

❗ Current head c63a253 differs from pull request most recent head 1979264. Consider uploading reports for the commit 1979264 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@               Coverage Diff               @@
##              master     #1618       +/-   ##
===============================================
+ Coverage     69.856%   70.279%   +0.423%     
- Complexity      9695      9907      +212     
===============================================
  Files            703       705        +2     
  Lines          37772     38427      +655     
  Branches        6139      6275      +136     
===============================================
+ Hits           26386     27006      +620     
- Misses          8929      8945       +16     
- Partials        2457      2476       +19     
Files Coverage Δ
.../samtools/cram/compression/ExternalCompressor.java 74.074% <100.000%> (ø)
...tools/cram/compression/RANSExternalCompressor.java 76.000% <100.000%> (+5.167%) ⬆️
...tools/cram/compression/rans/ArithmeticDecoder.java 100.000% <100.000%> (ø)
...jdk/samtools/cram/compression/rans/RANSDecode.java 100.000% <100.000%> (ø)
...ools/cram/compression/rans/RANSDecodingSymbol.java 100.000% <100.000%> (ø)
...jdk/samtools/cram/compression/rans/RANSParams.java 100.000% <100.000%> (ø)
...s/cram/compression/rans/rans4x8/RANS4x8Params.java 100.000% <100.000%> (ø)
...cram/compression/rans/ransnx16/RANSNx16Params.java 100.000% <100.000%> (ø)
...s/cram/structure/CompressionHeaderEncodingMap.java 85.621% <100.000%> (ø)
...tsjdk/samtools/cram/structure/CompressorCache.java 92.000% <100.000%> (+0.696%) ⬆️
... and 8 more

... and 25 files with indirect coverage changes

@yash-puligundla
Copy link
Contributor Author

Hi @cmnbroad I have addressed all the feedback so far. Please let me know if you notice anything. Thanks!

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another checkpoint of what I have so far.

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not even close to done, but checkpointing what I have - will resume when I get back from vacation.

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a little bit more to go, but almost done.

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this round I mostly focused on the decoders - many of my comments apply to the encoders as well, but I didn't duplicate them there since there are quite a few. I'm stopping for now and will need to do another round once these issues are addressed.

@yash-puligundla
Copy link
Contributor Author

@cmnbroad Thank you for your comments. I have addressed all of them so far. Ready for the next round of review.

yash-puligundla and others added 26 commits March 5, 2024 14:31
…ests or ../htscodecs/tests depending on the env
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants