-
Notifications
You must be signed in to change notification settings - Fork 12
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
Use ByteBuffer wrapper instead of LSB protocol #290
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #290 +/- ##
==========================================
+ Coverage 88.28% 88.33% +0.04%
==========================================
Files 80 81 +1
Lines 6985 7028 +43
Branches 495 495
==========================================
+ Hits 6167 6208 +41
- Misses 323 325 +2
Partials 495 495 ☔ View full report in Codecov by Sentry. |
Added accidentally omitted docstrings. |
Added some code to benchmark the performance of the BCF reader, and I've confirmed that the changes don't degrade the performance of the BAM/BCF readers:
|
bab0ae8
to
2e027f0
Compare
Rebased onto the latest master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 and thank you for adding the benchmarking codes!
@ToshimitsuArai Just a reminder, if you have any feedback on the PR, feel free to let me know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize for being late reviewing, I just forget it and thank you for remind🙇♂️
LGTM and thanks for faster 👍
Thank you both for taking the time to review this! |
This PR is the first step of #289.
It replaces the use of
ByteBuffer
as an LSB protocol implementation with a new thin wrapper utility forByteBuffer
.The modification policy here is to use the bare
ByteBuffer
methods whenever possible. The new wrapper namespace only collects functions that do what a single method ofByteBuffer
cannot do. Most of the implementation code comes from the implementation of the LSB protocol.