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

Fix a bug in reading bgzipped VCF files #184

Merged
merged 2 commits into from
Dec 13, 2019
Merged

Fix a bug in reading bgzipped VCF files #184

merged 2 commits into from
Dec 13, 2019

Conversation

alumi
Copy link
Member

@alumi alumi commented Dec 11, 2019

Problem

cljam.io.vcf/read-variants throws java.lang.ClassCastException for bgzipped VCF files.

Cause

#180 introduced switching of reader type between java.io.BufferedReader and bgzf4j.BGZFInputStream but did not modify the existing code.

Changes

Switch methods based on the type of reader instance.

Tests

  • lein check
  • lein test :all
  • lein cloverage

@alumi alumi added the bug label Dec 11, 2019
@alumi alumi requested review from niyarin and a team December 11, 2019 03:46
@ghost ghost requested review from athos and yito88 and removed request for a team December 11, 2019 03:46
@alumi alumi removed the request for review from yito88 December 11, 2019 03:47
@codecov
Copy link

codecov bot commented Dec 11, 2019

Codecov Report

Merging #184 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #184      +/-   ##
==========================================
+ Coverage   86.47%   86.48%   +<.01%     
==========================================
  Files          75       75              
  Lines        5924     5926       +2     
  Branches      497      497              
==========================================
+ Hits         5123     5125       +2     
  Misses        304      304              
  Partials      497      497
Impacted Files Coverage Δ
src/cljam/io/vcf.clj 95.74% <ø> (ø) ⬆️
src/cljam/io/vcf/reader.clj 85.93% <100%> (+0.22%) ⬆️

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 faa8524...f4779bb. Read the comment docs.

Copy link
Contributor

@niyarin niyarin left a comment

Choose a reason for hiding this comment

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

LGTM

@alumi alumi assigned athos and unassigned niyarin Dec 11, 2019
Copy link
Member

@athos athos left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! As for the production code change, LGTM👍

As for the test code, it would be nice to have a comment emphasizing that the call to vcf/read-variants at L300 is important to verify this change, in order to prevent someone in the future from accidentally optimizing out that call from the test case. Actually, I didn't get how that test case would verify the change at the first glance 😅

Or alternatively, how about adding one more extra test case that looks like the following, and add a comment noting the above thing to it?

(with-open [v (vcf/reader test-vcf-complex-file)
            g (vcf/reader test-vcf-complex-gz-file)]
  (is (= (vcf/read-variants v)
         (vcf/read-variants g))))

@alumi
Copy link
Member Author

alumi commented Dec 13, 2019

@athos Thanks! I added an extra test case in f4779bb.

Copy link
Member

@athos athos left a comment

Choose a reason for hiding this comment

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

Thank you for updating! I will merge the changes 💪

@athos athos merged commit b192b1a into master Dec 13, 2019
@athos athos deleted the fix/bgzip-vcf-reader branch December 13, 2019 02:25
@alumi
Copy link
Member Author

alumi commented Dec 13, 2019

Thank you for reviewing @niyarin @athos!!

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.

3 participants