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.

Sign up for GitHub

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

Jump to bottom

Add support for random reading of a bcf file. #187

Merged
merged 3 commits into from
Jan 23, 2020
Merged

Add support for random reading of a bcf file. #187

merged 3 commits into from
Jan 23, 2020
+178 −87

Conversation

niyarin
Copy link
Contributor

@niyarin niyarin commented Jan 15, 2020

This PR provides random reader of BCF files with CSI.

@niyarin niyarin requested a review from alumi as a code owner January 15, 2020 08:46
@niyarin niyarin requested a review from a team January 15, 2020 08:46
@ghost ghost requested review from athos and removed request for a team January 15, 2020 08:46
@codecov
Copy link

codecov bot commented Jan 15, 2020 •
edited
Loading

Codecov Report

Merging #187 into master will increase coverage by 0.08%.
The diff coverage is 91.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
+ Coverage   86.59%   86.67%   +0.08%     
==========================================
  Files          76       76              
  Lines        6011     6051      +40     
  Branches      501      501              
==========================================
+ Hits         5205     5245      +40     
  Misses        305      305              
  Partials      501      501
Impacted Files Coverage Δ
src/cljam/io/protocols.clj 100% <ø> (ø) ⬆️
src/cljam/io/vcf.clj 95.83% <100%> (ø) ⬆️
src/cljam/io/vcf/reader.clj 86.66% <100%> (+0.09%) ⬆️
src/cljam/io/bcf/reader.clj 89.77% <91.37%> (+1.06%) ⬆️
src/cljam/io/csi.clj 100% <0%> (+1.72%) ⬆️
src/cljam/io/util/bin.clj 97.5% <0%> (+2.5%) ⬆️

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 1287b96...88d7eb0. Read the comment docs.

alumi
alumi requested changes Jan 22, 2020
View reviewed changes
Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

Thanks! 👍 LGTM except for a redefined var.

test/cljam/test_common.clj Outdated
(def test-bcf-various-bins-file "test-resources/bcf/various-bins.bcf")
(def test-bcf-various-bins-csi-file "test-resources/bcf/various-bins.bcf.csi")
(def test-large-bcf-file (cavia/resource mycavia "large.bcf"))
(def test-large-vcf-csi-file (cavia/resource mycavia "large.bcf.csi"))
Copy link
Member

Choose a reason for hiding this comment

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

Rename this to test-large-bcf-csi-file as test-large-vcf-csi-file is defined at https://github.com/chrovis/cljam/pull/187/files#diff-ec7848941b3c1273617402827ddb7e2dR220

Suggested change
(def test-large-vcf-csi-file (cavia/resource mycavia "large.bcf.csi"))
(def test-large-bcf-csi-file (cavia/resource mycavia "large.bcf.csi"))

@niyarin
Copy link
Contributor Author

niyarin commented Jan 22, 2020

Thank you for pointing.
I fixed the wrong variable name.

alumi
alumi approved these changes Jan 22, 2020
View reviewed changes
Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

👍

athos
athos reviewed Jan 22, 2020
View reviewed changes
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 feature enhancement! LGTM 👍
I just left a somewhat nitpicky comment 😅

src/cljam/io/bcf/reader.clj Outdated

(defn read-variants-randomly
"Reads variants of the bgzip compressed BCF file randomly using csi file.
Returning them as a lazy sequence."
Copy link
Member

Choose a reason for hiding this comment

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

Just a matter of English grammar, but it sounds more natural to me if the second sentence starts with Returns them as ... instead of Returning them as ..., unless you join these two sentences into one:

Suggested change
Returning them as a lazy sequence."
Returns them as a lazy sequence."

@niyarin
Copy link
Contributor Author

niyarin commented Jan 23, 2020

Thank you for pointing.
I fixed the comment.

athos
athos approved these changes Jan 23, 2020
View reviewed changes
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! I will merge this 💪

@athos athos merged commit e98b889 into master Jan 23, 2020
@athos athos deleted the feature/bcf-randomly-reader branch January 23, 2020 01:45
@niyarin
Copy link
Contributor Author

niyarin commented Jan 23, 2020

Thank you for reviewing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alumi alumi alumi approved these changes

@athos athos athos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@niyarin @athos @alumi