Add support for CSI reader. #182

niyarin · 2019-11-19T21:34:16Z

This PR adds support for CSI reader to read BCF file randomly.
I've modify functions for other index file so that CSI can be handled by io/util/bin.
The referenced specification is CSIV1.

codecov · 2019-11-19T21:38:48Z

Codecov Report

Merging #182 into master will increase coverage by 0.15%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
+ Coverage   86.48%   86.64%   +0.15%     
==========================================
  Files          75       76       +1     
  Lines        5926     6011      +85     
  Branches      497      499       +2     
==========================================
+ Hits         5125     5208      +83     
  Misses        304      304              
- Partials      497      499       +2

Impacted Files	Coverage Δ
src/cljam/io/vcf/reader.clj	`86.56% <100%> (+0.62%)`	⬆️
src/cljam/io/vcf.clj	`95.83% <100%> (+0.08%)`	⬆️
src/cljam/io/bam_index/core.clj	`80% <100%> (+1.05%)`	⬆️
src/cljam/io/bam_index/common.clj	`100% <100%> (ø)`	⬆️
src/cljam/io/tabix.clj	`100% <100%> (ø)`	⬆️
src/cljam/io/util/bin.clj	`95% <93.1%> (-0.84%)`	⬇️
src/cljam/io/csi.clj	`98.27% <98.27%> (ø)`

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 211fe8e...a29c922. Read the comment docs.

alumi

Thanks for working on this PR! I've added some comments. I think we need to check the implementation of get-spans carefully.

src/cljam/io/csi.clj

src/cljam/io/bam_index/core.clj

src/cljam/io/csi.clj

src/cljam/io/tabix.clj

test/cljam/io/csi_test.clj

src/cljam/io/util/bin.clj

alumi · 2019-11-20T07:07:50Z

src/cljam/io/util/bin.clj

+         (range (+
+                 t
+                 (bit-shift-right beg s))
+                (+ t 1 (bit-shift-right (- end 1) s)))))


Suggested change

(+ t 1 (bit-shift-right (- end 1) s)))))

(+ t 1 (bit-shift-right (dec end) s)))))

r6eve

Thank you for new CSI feature!
Sorry, I'm halfway through, I left some minor comments.

src/cljam/io/csi.clj

src/cljam/io/util/bin.clj

niyarin · 2019-11-25T23:39:22Z

Thank you for pointing.
I fixed them.

r6eve

Thank you for the fix.
I left comments about unused imports.

r6eve · 2019-11-26T03:42:12Z

src/cljam/io/util/bin.clj

@@ -1,44 +1,45 @@
 (ns cljam.io.util.bin
-  (:require [cljam.io.util.chunk :as util-chunk]))
+  (:require [cljam.io.util.chunk :as util-chunk])
+  (:import [cljam.io.util.chunk Chunk]))


Unused imports here. Using clj-kondo, we can catch that.

r6eve · 2019-11-26T03:43:07Z

test/cljam/io/csi_test.clj

+            [cljam.io.csi :as csi])
+  (:import
+   [cljam.io.csi CSI]
+   [cljam.io.util.chunk Chunk]))


r6eve

I've added more comments.

r6eve · 2019-11-26T05:38:18Z

src/cljam/io/csi.clj

+          (->> (filter #(< % (last bins)) oct-cumulative-sums)
+               last
+               inc)


oct-cumulative-sums is sorted by ascending order, so take-while is better.

(->> (take-while #(< % (last bins)) oct-cumulative-sums) last inc)

r6eve · 2019-11-26T05:38:56Z

src/cljam/io/csi.clj

+          target-bin
+          (first
+           (filter #(< min-index %) bins))


bins is also sorted.

target-bin (first (take-while #(< min-index %) bins))

r6eve · 2019-11-26T05:40:38Z

src/cljam/io/csi.clj

+          res (get (loffset ref-idx) target-bin)]
+      (if res res 0)))


This could be (get (loffset ref-idx) target-bin 0)

alumi · 2019-11-26T05:08:54Z

test/cljam/io/util/bin_test.clj

+                (util-bin/get-spans csi* 0 1 100000)))))
+
+(deftest reg->bins-test
+  (are [x] (= [1 9 73 585 4681]


Suggested change

(are [x] (= [1 9 73 585 4681]

(are [x] (= [0 1 9 73 585 4681]

alumi · 2019-11-26T06:05:22Z

src/cljam/io/csi.clj

+        refs (range n-ref)
+        bins (vec (repeatedly n-ref #(read-bin-index rdr)))
+        bidx (zipmap refs (map #(into {} (map (juxt :bin :chunks)) %) bins))
+        max-bin


Redundant line break

Suggested change

max-bin

max-bin (apply + (map #(bit-shift-left 1 (* % 3)) (range (inc depth))))

alumi · 2019-11-26T06:05:32Z

src/cljam/io/csi.clj

+        bidx (zipmap refs (map #(into {} (map (juxt :bin :chunks)) %) bins))
+        max-bin
+        (apply + (map #(bit-shift-left 1 (* % 3)) (range (inc depth))))
+        loffset


alumi · 2019-11-26T06:28:27Z

src/cljam/io/csi.clj

+  (get-chunks [_ ref-idx bins]
+    (into [] (mapcat (get bidx ref-idx) bins)))
+  (get-min-offset [_ ref-idx beg bins]
+    (let [oct-cumulative-sums


Redundant line break

Suggested change

(let [oct-cumulative-sums

(let [oct-cumulative-sums (->> (range (inc depth))

alumi · 2019-11-26T07:32:40Z

src/cljam/io/csi.clj

+  util-bin/IBinaryIndex
+  (get-chunks [_ ref-idx bins]
+    (into [] (mapcat (get bidx ref-idx) bins)))
+  (get-min-offset [_ ref-idx beg bins]


I'm sorry I forgot that CSI doesn't always store l-offsets for all bins like BAI.
So in this comment #182 (comment), when bin 12 doesn't exist in CSI, we have to check the left bin 11 and then the parent bin 5 ... recursively until we find an existing bin.
Note that the bin 11 is not contained in the result of reg->bins in this case.
The current implementation will return the l-offset of the bin 13 if 12 doesn't exist, which results in unintentionally filtering necessary chunks out.

niyarin · 2019-11-26T23:37:53Z

Thank you for pointing.
I fixed them.

alumi

Added some more trivial comments on styling issues.

alumi · 2019-11-27T00:39:49Z

src/cljam/io/tabix.clj

-    (get (get (.lidx this) ref-idx)
+  util-bin/IBinningIndex
+  (get-chunks [_ ref-idx bins]
+    (into [] (mapcat (get bidx ref-idx) bins)))


Use mapcat transducer

Suggested change

(into [] (mapcat (get bidx ref-idx) bins)))

(into [] (mapcat (get bidx ref-idx)) bins))

or vec

Suggested change

(into [] (mapcat (get bidx ref-idx) bins)))

(vec (mapcat (get bidx ref-idx) bins)))

https://github.com/bbatsov/clojure-style-guide#to-vector

alumi · 2019-11-27T00:42:31Z

src/cljam/io/bam_index/core.clj

-         (util-bin/pos->lidx-offset beg common/linear-index-shift) 0)))
+  util-bin/IBinningIndex
+  (get-chunks [_ ref-idx bins]
+    (into [] (mapcat (get bidx ref-idx) bins)))


Use mapcat transducer

Suggested change

(into [] (mapcat (get bidx ref-idx) bins)))

(into [] (mapcat (get bidx ref-idx)) bins))

or vec

Suggested change

(into [] (mapcat (get bidx ref-idx) bins)))

(vec (mapcat (get bidx ref-idx) bins)))

https://github.com/bbatsov/clojure-style-guide#to-vector

alumi · 2019-11-27T00:43:27Z

src/cljam/io/util/bin.clj

+           (fn [^long d]
+             (let [t
+                   (apply + (map (fn [^long x] (bit-shift-left 1 (* x 3)))
+                                 (range (+ d 1))))


Suggested change

(range (+ d 1))))

(range (inc d))))

alumi · 2019-11-27T00:43:37Z

src/cljam/io/util/bin.clj

+                                 (range (+ d 1))))
+                   s (+ min-shift (* 3 (- depth d 1)))]
+               (range (+ t (bit-shift-right beg s))
+                      (+ t 1 (bit-shift-right (- end 1) s))))))


Suggested change

(+ t 1 (bit-shift-right (- end 1) s))))))

(+ t 1 (bit-shift-right (dec end) s))))))

alumi · 2019-11-27T00:46:30Z

src/cljam/io/csi.clj

+(deftype CSI [n-ref min-shift depth bidx loffset]
+  util-bin/IBinningIndex
+  (get-chunks [_ ref-idx bins]
+    (into [] (mapcat (get bidx ref-idx) bins)))


Use mapcat transducer

Suggested change

(into [] (mapcat (get bidx ref-idx) bins)))

(into [] (mapcat (get bidx ref-idx)) bins))

or vec

Suggested change

(into [] (mapcat (get bidx ref-idx) bins)))

(vec (mapcat (get bidx ref-idx) bins)))

https://github.com/bbatsov/clojure-style-guide#to-vector

alumi · 2019-11-27T01:52:59Z

src/cljam/io/csi.clj

+                         last)]
+      (or (->> bins
+               (drop-while #(>  min-index %))
+               (keep #(get (loffset ref-idx) %))


Suggested change

(keep #(get (loffset ref-idx) %))

(keep (loffset ref-idx))

alumi · 2019-11-27T04:31:38Z

src/cljam/io/csi.clj

+    (let [oct-cumulative-sums (->> (range (inc depth))
+                                   (map #(bit-shift-left 1 (* % 3)))
+                                   (reductions +))
+          min-index (->> (take-while #(<= % (last bins)) oct-cumulative-sums)


Prefer (foo bar) over a threading macro with only 2 args (->> bar foo)

Suggested change

min-index (->> (take-while #(<= % (last bins)) oct-cumulative-sums)

min-index (last (take-while #(<= % (last bins)) oct-cumulative-sums))

or more concisely

(let [min-index (->> (range (inc depth)) (map #(bit-shift-left 1 (* % 3))) (reductions +) (take-while #(<= % (last bins))) last)]

r6eve · 2019-11-27T09:32:08Z

src/cljam/io/csi.clj

+          min-index (->> (take-while #(<= % (last bins)) oct-cumulative-sums)
+                         last)]
+      (or (->> bins
+               (drop-while #(>  min-index %))


Extra spaces between > and min-index. And partial is preferable. https://guide.clojure.style/#partial

Suggested change

(drop-while #(> min-index %))

(drop-while (partial > min-index))

alumi · 2019-12-02T05:00:39Z

@niyarin Would you rebase this branch onto master (faa8524)? CI will fail in this branch. See #183 for more details.

niyarin · 2019-12-16T22:59:11Z

Sorry for my late.
I fixed them.

r6eve

Thanks for fixing. I left more minor comments.

r6eve · 2019-12-17T09:29:27Z

src/cljam/io/util/bin.clj

+(defn bin-beg ^long [^long bin ^long min-shift ^long depth]
+  (let [level (bin-level bin)]
+    (inc (* (- bin (first-bin-of-level level))
+            (bin-width-of-level level min-shift depth)))))

 (def ^:private reg->bins (memoize reg->bins*))


The position of defining memoized version is a little too far from original version.

r6eve · 2019-12-17T09:37:21Z

src/cljam/io/csi.clj

+                                     (comp (map (juxt :bin :loffset))
+                                           (filter #(<= (first %) max-bin))
+                                           (map
+                                            #(vector
+                                              (util-bin/bin-beg
+                                               (first %)
+                                               min-shift
+                                               depth)
+                                              (second %))))


Prefer literal collection syntax https://guide.clojure.style/#literal-col-syntax

(comp (map (juxt :bin :loffset)) (filter #(<= (first %) max-bin)) (map (fn [[bin loffset]] [(util-bin/bin-beg bin min-shift depth) loffset])))

r6eve · 2019-12-17T09:38:29Z

test/cljam/io/util/bin_test.clj

+      (is (=  1 (util-bin/bin-beg 4681 index-shift 5)))
+      (is (=  (+ 1 min-size) (util-bin/bin-beg 4682 index-shift 5)))
+      (is (=  (+ 1 (* min-size 2)) (util-bin/bin-beg 4683 index-shift 5)))
+      (is (=  1 (util-bin/bin-beg 585 14 5)))
+      (is (=  (+ 1 (* min-size 8)) (util-bin/bin-beg 586 index-shift 5)))
+      (is (=  (+ 1 (* min-size 16)) (util-bin/bin-beg 587 index-shift 5)))
+      (is (=  (+ 1 (* min-size 24)) (util-bin/bin-beg 588 index-shift 5))))))


Extra spaces here.

r6eve · 2019-12-18T04:59:16Z

src/cljam/io/csi.clj

+
+(defn- read-chunks!
+  [rdr]
+  (->> #(Chunk. (lsb/read-long rdr) (lsb/read-long rdr))


Prefer constructor functions. https://guide.clojure.style/#record-constructors

r6eve · 2019-12-18T10:23:08Z

src/cljam/io/csi.clj

+  (->> #(hash-map
+         :bin (lsb/read-int rdr)
+         :loffset (lsb/read-long rdr)
+         :chunks (read-chunks! rdr))


In current implementation of Clojure, function arguments are certainly evaluated from left to right, but it's not very well known style.
I think using let form before hash-map might be more reasonable for side-effects. Please correct me if I'm wrong.
https://clojure.org/reference/evaluation

niyarin · 2019-12-20T07:22:46Z

Thank you for pointing out and suggestions.
I fixed them.

r6eve · 2019-12-20T09:26:44Z

src/cljam/io/csi.clj

+
+(defn- read-chunks!
+  [rdr]
+  (->> #(chunk/->Chunk (lsb/read-long rdr) (lsb/read-long rdr))


Sorry for nagging, would you use let form for side effects?

niyarin · 2019-12-23T02:08:34Z

I'm sorry to overlook.
As a small fix, I added it to the previous commit.

alumi

Thanks for the update 👍 The seeking logic looks good now. Added some trivial comments.

src/cljam/io/csi.clj

src/cljam/io/util/bin.clj

test/cljam/io/vcf_test.clj

src/cljam/io/vcf/reader.clj

niyarin · 2019-12-23T07:51:36Z

Thank you for pointing.
I fixed them.

alumi

LGTM 👍

r6eve

LGTM.
Would you squash the commits into a single one? Then I'll merge it.

niyarin · 2019-12-23T10:18:34Z

Thank you for reviewing! @r6eve @alumi
And I squashed the commits.

r6eve · 2019-12-23T10:22:48Z

Thank you so much for your cooperation!

niyarin requested a review from alumi as a code owner November 19, 2019 21:34

niyarin requested a review from a team November 19, 2019 21:34

ghost requested review from r6eve and removed request for a team November 19, 2019 21:34

alumi assigned alumi and r6eve Nov 20, 2019

alumi added the enhancement label Nov 20, 2019

alumi requested changes Nov 20, 2019

View reviewed changes

alumi reviewed Nov 20, 2019

View reviewed changes

r6eve reviewed Nov 20, 2019

View reviewed changes

src/cljam/io/csi.clj Outdated Show resolved Hide resolved

src/cljam/io/csi.clj Outdated Show resolved Hide resolved

src/cljam/io/util/bin.clj Outdated Show resolved Hide resolved

niyarin force-pushed the feature/csi-reader branch 2 times, most recently from 2d54548 to 9f3df7a Compare November 25, 2019 23:30

r6eve reviewed Nov 26, 2019

View reviewed changes

alumi reviewed Nov 26, 2019

View reviewed changes

alumi reviewed Nov 27, 2019

View reviewed changes

r6eve reviewed Nov 27, 2019

View reviewed changes

niyarin force-pushed the feature/csi-reader branch 5 times, most recently from e6b11b6 to 0706f5c Compare December 16, 2019 22:51

r6eve reviewed Dec 17, 2019

View reviewed changes

r6eve reviewed Dec 18, 2019

View reviewed changes

r6eve reviewed Dec 20, 2019

View reviewed changes

niyarin force-pushed the feature/csi-reader branch 2 times, most recently from c0387f0 to 4d08f46 Compare December 20, 2019 18:15

alumi reviewed Dec 23, 2019

View reviewed changes

niyarin force-pushed the feature/csi-reader branch from 474d681 to 2c5dd95 Compare December 23, 2019 07:50

niyarin force-pushed the feature/csi-reader branch from 2c5dd95 to be67998 Compare December 23, 2019 07:58

alumi approved these changes Dec 23, 2019

View reviewed changes

r6eve approved these changes Dec 23, 2019

View reviewed changes

niyarin force-pushed the feature/csi-reader branch from be67998 to a29c922 Compare December 23, 2019 10:15

Add support for csi reader.

a29c922

r6eve merged commit d99122a into master Dec 23, 2019

r6eve deleted the feature/csi-reader branch December 23, 2019 10:23

	(+ t 1 (bit-shift-right (- end 1) s)))))
	(+ t 1 (bit-shift-right (dec end) s)))))

	(are [x] (= [1 9 73 585 4681]
	(are [x] (= [0 1 9 73 585 4681]

	max-bin
	max-bin (apply + (map #(bit-shift-left 1 (* % 3)) (range (inc depth))))

	(let [oct-cumulative-sums
	(let [oct-cumulative-sums (->> (range (inc depth))

	(into [] (mapcat (get bidx ref-idx) bins)))
	(into [] (mapcat (get bidx ref-idx)) bins))

	(into [] (mapcat (get bidx ref-idx) bins)))
	(vec (mapcat (get bidx ref-idx) bins)))

	min-index (->> (take-while #(<= % (last bins)) oct-cumulative-sums)
	min-index (last (take-while #(<= % (last bins)) oct-cumulative-sums))

	(drop-while #(> min-index %))
	(drop-while (partial > min-index))

Add support for CSI reader. #182

Add support for CSI reader. #182

Conversation

niyarin commented Nov 19, 2019

codecov bot commented Nov 19, 2019 • edited

Codecov Report

alumi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r6eve left a comment

Choose a reason for hiding this comment

niyarin commented Nov 25, 2019

r6eve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r6eve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alumi Nov 26, 2019 • edited

Choose a reason for hiding this comment

niyarin commented Nov 26, 2019

alumi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alumi commented Dec 2, 2019

niyarin commented Dec 16, 2019

r6eve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niyarin commented Dec 20, 2019

Choose a reason for hiding this comment

niyarin commented Dec 23, 2019

alumi left a comment

Choose a reason for hiding this comment

niyarin commented Dec 23, 2019

alumi left a comment

Choose a reason for hiding this comment

r6eve left a comment

Choose a reason for hiding this comment

niyarin commented Dec 23, 2019

r6eve commented Dec 23, 2019

codecov bot commented Nov 19, 2019 •

edited

alumi Nov 26, 2019 •

edited