Add support for generating CSI file from bgzip compressed VCF. by niyarin · Pull Request #190 · chrovis/cljam

niyarin · 2020-02-26T11:38:02Z

Add support for generating CSI file from VCF.

athos

Thanks for working on another feature! I just added some comments on relatively superficial matters.

codecov · 2020-03-02T06:31:01Z

Codecov Report

Merging #190 into master will increase coverage by 0.14%.
The diff coverage is 92.22%.

@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
+ Coverage   86.67%   86.81%   +0.14%     
==========================================
  Files          76       77       +1     
  Lines        6057     6205     +148     
  Branches      502      516      +14     
==========================================
+ Hits         5250     5387     +137     
+ Misses        305      302       -3     
- Partials      502      516      +14

Impacted Files	Coverage Δ
src/cljam/io/protocols.clj	`100.00% <ø> (ø)`
src/cljam/io/csi.clj	`91.46% <89.06%> (-8.54%)`	⬇️
src/cljam/io/util/bin.clj	`94.33% <93.33%> (-3.17%)`	⬇️
src/cljam/algo/vcf_indexer.clj	`100.00% <100.00%> (ø)`
src/cljam/io/bam_index/writer.clj	`84.37% <100.00%> (+0.08%)`	⬆️
src/cljam/io/sam/util.clj	`92.30% <100.00%> (+11.22%)`	⬆️
src/cljam/io/vcf.clj	`96.00% <100.00%> (+0.16%)`	⬆️
src/cljam/io/vcf/reader.clj	`88.75% <100.00%> (+2.08%)`	⬆️

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 6d961d7...2444372. Read the comment docs.

niyarin · 2020-03-02T07:40:09Z

Thank you for reviewing.
I fixed bad expressions and added some test cases about vcf not ascending order chroms.

alumi

Thanks for the updates! I added a few more comments.

niyarin · 2020-03-16T01:23:02Z

Sorry for late, I fixed.

alumi

Looks like lein test :all is failing 🔥

niyarin · 2020-03-17T08:27:47Z

I'm so sorry,I will fix test cases same as others.

athos

Added some more trivial comments.

athos · 2020-03-18T05:39:46Z

+              [bin (->> offsets
+                        (map #(chunk/->Chunk (:file-beg %) (:file-end %)))
+                        reverse)]))
+       sort


Suggested change

sort

athos · 2020-03-18T05:40:07Z

+  [offsets shift depth]
+  (let [chr-offsets (->> (partition-by :chr offsets)
+                         (map #(vector (:chr (first %)) %))
+                         sort


Suggested change

sort

athos · 2020-03-18T05:43:08Z

+   from offsets {:file-beg :file-end :beg :end :chr }"
+  [offsets shift depth]
+  (let [chr-offsets (->> (partition-by :chr offsets)
+                         (map #(vector (:chr (first %)) %))


These two lines seem to be equivalent to (group-by :chr offsets) in this case.

athos · 2020-03-18T05:44:51Z

+
+(defn create-index
+  "Creates a CSI index file from the VCF file."
+  [in-bgziped-vcf out-csi {:keys [shift depth] :or {shift 14 depth 5}}]


s/bgziped/bgzipped/

niyarin · 2020-03-23T07:33:11Z

Thank you for pointing.

I made the following changes.

Loffset in CSI decided not to match the implementation of hts_lib.
- Specifically, the loffset of the key that does not exist during the beg-end of vcf came closer to the target data.
Added an operation to move smaller chunks to higher binning in bidx calculation,same as hts_lib.

alumi

Run formatter and linters every time before you push your commits
It seems like generated CSI files are not compatible with bcftools
- Please make sure that both cljam.io.vcf.read-variants-randomly and bcftools view works as well
Add a test case for VCF files with skipped contigs as in #190 (comment)

niyarin · 2020-03-24T09:49:30Z

Sorry, I applied linter.

niyarin · 2020-03-25T02:37:51Z

Bcftools (hts_lib) will generate an error if some tabix data (such as format,col_seq , names and so on) is not inserted into CSI aux.
So I fixed CSI for portability.

alumi

Thanks! The indexing looks good. Added some more trivial comments.

niyarin · 2020-04-06T05:10:29Z

Thank you for pointing.I fixed them.
I think that arguments of reg->bin is [beg,end) (one-based, half-close-half-open).

alumi

Thank you for the changes. I've added some suggestions around reg->bin. Would you check them, please?

alumi · 2020-04-09T07:10:06Z

@@ -68,4 +48,4 @@
  [aln]
  (let [beg (dec (:pos aln))


Suggested change

(let [beg (dec (:pos aln))

(let [beg (:pos aln)

alumi · 2020-04-09T07:11:09Z

+  (let [bin (util-bin/reg->bin (.pos aln) (inc (.end aln))
+                               linear-index-shift linear-index-depth)


Suggested change

(let [bin (util-bin/reg->bin (.pos aln) (inc (.end aln))

linear-index-shift linear-index-depth)

(let [bin (util-bin/reg->bin

(.pos aln) (.end aln) linear-index-shift linear-index-depth)

alumi · 2020-04-09T07:11:50Z

+        beg (Math/min max-pos (Math/max 0 (dec beg)))
+        end (Math/min max-pos (Math/max 0 (dec end)))]


Suggested change

beg (Math/min max-pos (Math/max 0 (dec beg)))

end (Math/min max-pos (Math/max 0 (dec end)))]

beg (dec (Math/max 1 (Math/min max-pos beg)))

end (dec (Math/max 1 (Math/min max-pos end)))]

alumi · 2020-04-09T07:13:26Z

+(deftest reg->bin-test
+  (is (= (util-bin/reg->bin 1 1 14 6) 37449))
+  (is (= (util-bin/reg->bin 1 1 14 7) 299593))
+  (is (= (util-bin/reg->bin 1 32769 15 6) 4681))
+  (is (= (util-bin/reg->bin 1 2 0 2) 1))
+  (is (= (util-bin/reg->bin 240877561 240877568 14 6) 52150)))


Suggested change

(deftest reg->bin-test

(is (= (util-bin/reg->bin 1 1 14 6) 37449))

(is (= (util-bin/reg->bin 1 1 14 7) 299593))

(is (= (util-bin/reg->bin 1 32769 15 6) 4681))

(is (= (util-bin/reg->bin 1 2 0 2) 1))

(is (= (util-bin/reg->bin 240877561 240877568 14 6) 52150)))

(deftest reg->bin-test

(testing "BAI-compatible"

(are [?start ?end ?bin]

(= ?bin (util-bin/reg->bin ?start ?end 14 5))

0 0 4681

0 1 4681

1 1 4681

1 16384 4681

16384 16384 4681

16384 16385 585

16385 16385 4682

131072 131072 4688

131072 131073 73

8388608 8388609 1

67108864 67108865 0

536870912 536870912 37448

536870912 536870913 37448))

(testing "1-by-1"

(are [?start ?end ?bin]

(= ?bin (util-bin/reg->bin ?start ?end 0 2))

0 1 9

1 1 9

2 2 10

1 2 1

1 8 1

1 9 0

8 9 0

9 9 17

63 64 8

64 64 72

64 64 72))

(testing "various min-shfits and depths"

(is (= (util-bin/reg->bin 1 1 14 6) 37449))

(is (= (util-bin/reg->bin 1 1 14 7) 299593))

(is (= (util-bin/reg->bin 1 32769 15 6) 4681))

(is (= (util-bin/reg->bin 240877561 240877568 14 6) 52150))))

niyarin · 2020-04-10T07:50:03Z

Thank you,I fixed.

alumi

LGTM 👍

I found a bug in the random reading of VCF files. Could you fix it in another PR?

;; with test-resources/vcf/test-chr-skipped.vcf.gz.csi
(with-open [r (vcf/reader "test-resources/vcf/test-chr-skipped.vcf.gz")]
        (doall (vcf/read-variants-randomly r {:chr "chr3"} {})))
Execution error (NullPointerException) at cljam.io.csi.CSI/get_chunks (csi.clj:19).

alumi · 2020-04-14T05:17:01Z

@niyarin Please don't forget to squash the commits before merging this branch 😀

athos

@niyarin Thanks for your persistent effort to improve the code for this feature! 🙏Now the implementation works fine 👍

Just one more thing, can you add a mention to CSI, as well as Tabix, in the docstring of cljam.io.vcf/read-variants-randomly as another supported index format? The current description looks to me like it only supports the Tabix format. Also, you may want to avoid using the name tabix-data in the body of that function.

Once you make a commit for the above point and squash all the commits into one (or a few), I will merge the patch soon.

niyarin · 2020-04-16T10:55:31Z

Thank you,I added a comment that VCF can also use CSI and renamed 'tabix-data' to 'index-data'.

alumi · 2020-04-17T02:31:19Z

🎉 🎉 🎉

niyarin requested a review from alumi as a code owner February 26, 2020 11:38

niyarin requested a review from a team February 26, 2020 11:38

alumi requested review from a team and athos and removed request for a team February 27, 2020 09:20

alumi requested changes Feb 28, 2020

View reviewed changes

athos requested changes Feb 28, 2020

View reviewed changes

Comment thread src/cljam/io/util/bin.clj Outdated

Comment thread src/cljam/io/util/bin.clj Outdated

Comment thread src/cljam/io/vcf.clj Outdated

Comment thread src/cljam/algo/vcf_indexer.clj Outdated

alumi assigned niyarin Mar 2, 2020

alumi added the enhancement label Mar 2, 2020

niyarin force-pushed the feature/csi-writer-for-vcf branch from 9ae9ce7 to 803b2d5 Compare March 2, 2020 06:59

alumi requested changes Mar 2, 2020

View reviewed changes

Comment thread src/cljam/io/vcf.clj Outdated

Comment thread src/cljam/io/util/bin.clj Outdated

Comment thread test/cljam/io/util/bin_test.clj Outdated

Comment thread test/cljam/algo/vcf_indexer_test.clj Outdated

alumi requested changes Mar 3, 2020

View reviewed changes

Comment thread src/cljam/io/csi.clj Outdated

Comment thread src/cljam/algo/vcf_indexer.clj Outdated

Comment thread src/cljam/io/csi.clj Outdated

Comment thread src/cljam/io/vcf/reader.clj Outdated

Comment thread src/cljam/io/csi.clj Outdated

Comment thread test/cljam/algo/vcf_indexer_test.clj

niyarin force-pushed the feature/csi-writer-for-vcf branch from cf1ce51 to cddf8c2 Compare March 16, 2020 01:26

alumi requested changes Mar 17, 2020

View reviewed changes

Comment thread src/cljam/io/csi.clj Outdated

Comment thread test/cljam/algo/vcf_indexer_test.clj Outdated

Comment thread test/cljam/io/csi_test.clj Outdated

athos reviewed Mar 18, 2020

View reviewed changes

niyarin force-pushed the feature/csi-writer-for-vcf branch 3 times, most recently from d55f8f2 to 79451d1 Compare March 23, 2020 07:18

alumi requested changes Mar 24, 2020

View reviewed changes

niyarin force-pushed the feature/csi-writer-for-vcf branch from 79451d1 to 3bf6899 Compare March 24, 2020 09:47

niyarin force-pushed the feature/csi-writer-for-vcf branch from e655e42 to 4fd65bd Compare March 25, 2020 02:29

alumi reviewed Apr 1, 2020

View reviewed changes

niyarin force-pushed the feature/csi-writer-for-vcf branch 2 times, most recently from 5d3a3aa to 985fed3 Compare April 6, 2020 02:39

alumi requested changes Apr 9, 2020

View reviewed changes

alumi approved these changes Apr 14, 2020

View reviewed changes

athos reviewed Apr 16, 2020

View reviewed changes

niyarin force-pushed the feature/csi-writer-for-vcf branch from 2c71be5 to 2444372 Compare April 16, 2020 10:32

Add support for generating CSI file from bgzip compressed VCF.

2444372

athos approved these changes Apr 17, 2020

View reviewed changes

athos merged commit fa2fa54 into master Apr 17, 2020

athos deleted the feature/csi-writer-for-vcf branch April 17, 2020 02:01

niyarin mentioned this pull request May 18, 2020

Add support for generating CSI file from BCF. #196

Merged

		(let [bin (util-bin/reg->bin (.pos aln) (inc (.end aln))
		linear-index-shift linear-index-depth)

		beg (Math/min max-pos (Math/max 0 (dec beg)))
		end (Math/min max-pos (Math/max 0 (dec end)))]

-(deftest reg->bin-test
-  (is (= (util-bin/reg->bin 1 1 14 6) 37449))
-  (is (= (util-bin/reg->bin 1 1 14 7) 299593))
-  (is (= (util-bin/reg->bin 1 32769 15 6) 4681))
-  (is (= (util-bin/reg->bin 1 2 0 2) 1))
-  (is (= (util-bin/reg->bin 240877561 240877568 14 6) 52150)))
+(deftest reg->bin-test
+  (testing "BAI-compatible"
+    (are [?start ?end ?bin]
+        (= ?bin (util-bin/reg->bin ?start ?end 14 5))
+0 4681
+1 4681
+1 4681
+16384 4681
+16384 4681
+16385 585
+16385 4682
+131072 4688
+131073 73
+      8388608 8388609 1
+      67108864 67108865 0
+      536870912 536870912 37448
+      536870912 536870913 37448))
+  (testing "1-by-1"
+    (are [?start ?end ?bin]
+        (= ?bin (util-bin/reg->bin ?start ?end 0 2))
+1 9
+1 9
+2 10
+2 1
+8 1
+9 0
+9 0
+9 17
+64 8
+64 72
+64 72))
+  (testing "various min-shfits and depths"
+    (is (= (util-bin/reg->bin 1 1 14 6) 37449))
+    (is (= (util-bin/reg->bin 1 1 14 7) 299593))
+    (is (= (util-bin/reg->bin 1 32769 15 6) 4681))
+    (is (= (util-bin/reg->bin 240877561 240877568 14 6) 52150))))

Conversation

niyarin commented Feb 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

athos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niyarin commented Mar 2, 2020

alumi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niyarin commented Mar 16, 2020

alumi left a comment

Choose a reason for hiding this comment

niyarin commented Mar 17, 2020

Uh oh!

Uh oh!

Uh oh!

athos left a comment

Choose a reason for hiding this comment

athos Mar 18, 2020

Choose a reason for hiding this comment

athos Mar 18, 2020

Choose a reason for hiding this comment

athos Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

athos Mar 18, 2020

Choose a reason for hiding this comment

niyarin commented Mar 23, 2020

alumi left a comment

Choose a reason for hiding this comment

niyarin commented Mar 24, 2020

niyarin commented Mar 25, 2020

alumi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niyarin commented Apr 6, 2020

alumi left a comment

Choose a reason for hiding this comment

alumi Apr 9, 2020

Choose a reason for hiding this comment

alumi Apr 9, 2020

Choose a reason for hiding this comment

alumi Apr 9, 2020

Choose a reason for hiding this comment

alumi Apr 9, 2020

Choose a reason for hiding this comment

niyarin commented Apr 10, 2020

alumi left a comment

codecov Bot commented Mar 2, 2020 •

edited

Loading

athos Mar 18, 2020 •

edited

Loading

athos left a comment •

edited

Loading

niyarin commented Apr 16, 2020 •

edited

Loading