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

Add support for nclist. #211

Merged
merged 3 commits into from Oct 15, 2020
Merged

Add support for nclist. #211

merged 3 commits into from Oct 15, 2020

Conversation

niyarin
Copy link
Contributor

@niyarin niyarin commented Sep 24, 2020

This pr adds support for Nested Containment List (NCList).

  • Add a second argument to "make-sorted-map-intervals" to select the data structure.
  • In the original, it is represented by two lists, but in order to represent it in Clojure, it is represented by one nested list.

@niyarin niyarin requested review from alumi and a team as code owners September 24, 2020 05:42
@niyarin niyarin requested review from athos and removed request for a team September 24, 2020 05:42
@codecov
Copy link

codecov bot commented Sep 24, 2020

Codecov Report

Merging #211 into master will increase coverage by 1.50%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
+ Coverage   87.23%   88.74%   +1.50%     
==========================================
  Files          78       76       -2     
  Lines        6347     6292      -55     
  Branches      511      436      -75     
==========================================
+ Hits         5537     5584      +47     
+ Misses        299      272      -27     
+ Partials      511      436      -75     
Impacted Files Coverage Δ
src/cljam/io/vcf/util.clj 93.28% <0.00%> (-2.04%) ⬇️
src/cljam/util/sequence.clj 78.94% <0.00%> (-2.01%) ⬇️
src/cljam/io/sam/util/sequence.clj 92.00% <0.00%> (-0.16%) ⬇️
src/cljam/io/twobit/writer.clj 80.95% <0.00%> (-0.15%) ⬇️
src/cljam/io/vcf/util/normalize.clj 98.18% <0.00%> (-0.05%) ⬇️
src/cljam/algo/pileup.clj 96.63% <0.00%> (-0.04%) ⬇️
src/cljam/io/sam/util/flag.clj 100.00% <0.00%> (ø)
src/cljam/util/whole_genome.clj
src/cljam/io/util.clj 98.48% <0.00%> (+0.02%) ⬆️
src/cljam/io/util/bgzf/gzi.clj 93.54% <0.00%> (+0.21%) ⬆️
... and 10 more

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 e6c8358...8dcbf25. Read the comment docs.

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.

Sorry for my late reply 🙏 and thank you for your PR!!

src/cljam/util/intervals.clj Outdated Show resolved Hide resolved
src/cljam/util/intervals.clj Outdated Show resolved Hide resolved
src/cljam/util/intervals.clj Outdated Show resolved Hide resolved
src/cljam/util/intervals.clj Outdated Show resolved Hide resolved
src/cljam/util/intervals.clj Outdated Show resolved Hide resolved
@niyarin
Copy link
Contributor Author

niyarin commented Oct 5, 2020

Thank you for pointing. I fixed them.

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.

LGTM 👍 👍 Thank you for the update!

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 adding this useful feature! 🙏 Added a comment.

Comment on lines 21 to 28
(when-let [target-intervals
(->> (subseq nclist >= start)
(map second)
(take-while #(<= (:start (first %)) end)))]
(mapcat #(cons (first %)
(find-nclist-overlap-intervals (second %)
start end))
target-intervals)))
Copy link
Member

Choose a reason for hiding this comment

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

Although it wouldn't cause a real problem, the conditional of this when-let will always be evaluated to a truthy value since take-while always returns a non-nil value. You can rewrite this as:

(some->> (subseq nclist >= start)
         (map second)
         (take-while #(<= (:start (first %)) end))
         seq
         (mapcat #(cons (first %)
                        (find-nclist-overlap-intervals (second %)
                                                       start end))))

Or simply:

(->> (subseq nclist >= start)
     (map second)
     (take-while #(<= (:start (first %)) end))
     (mapcat #(cons (first %)
                    (find-nclist-overlap-intervals (second %)
                                                   start end))))

@niyarin
Copy link
Contributor Author

niyarin commented Oct 14, 2020

I removed unnecessary when-let.

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.

LGTM 👍 Thanks again for working on this feature!

@athos athos merged commit 7e2e34c into master Oct 15, 2020
@athos athos deleted the feature/nclist-intervals branch October 15, 2020 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants