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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract region utils to cljam.util.region and add some functions. #110

Merged
merged 1 commit into from Sep 11, 2017

Conversation

alumi
Copy link
Member

@alumi alumi commented Sep 11, 2017

Summary

  • Extract region utils in cljam.util to cljam.util.region
  • Add merge-regions, subtract-region and complement-regions

Tests

  • lein test :all 馃啑

@codecov
Copy link

codecov bot commented Sep 11, 2017

Codecov Report

Merging #110 into master will increase coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #110      +/-   ##
=========================================
+ Coverage   85.28%   85.5%   +0.22%     
=========================================
  Files          61      62       +1     
  Lines        4029    4085      +56     
  Branches      400     399       -1     
=========================================
+ Hits         3436    3493      +57     
  Misses        193     193              
+ Partials      400     399       -1
Impacted Files Coverage 螖
src/cljam/util.clj 94.28% <酶> (-1.49%) 猬囷笍
src/cljam/util/region.clj 100% <100%> (酶)
src/cljam/tools/cli.clj 53.57% <100%> (酶) 猬嗭笍
src/cljam/algo/depth.clj 73.8% <100%> (酶) 猬嗭笍

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 2e1a4d1...cbe4dd7. Read the comment docs.

Copy link
Member

@totakke totakke 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 refactoring and additional convenient functions. I added a few comments. Please check them.

@@ -16,7 +16,8 @@
[cljam.algo.pileup :as plp]
[cljam.algo.convert :as convert]
[cljam.algo.level :as level]
[cljam.util :as util])
[cljam.util :as util]
Copy link
Member

Choose a reason for hiding this comment

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

cljam.util seems no more needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Removed the requirement.

(do (vreset! last-reg x) r)))))))
([^long max-gap-merged regs]
(merge-regions max-gap-merged regs #(update %1 :end max (:end %2))))
([^long max-gap-merged regs merge-fn]
Copy link
Member

Choose a reason for hiding this comment

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

I think merge-fn should be located in front like clojure.core/merge-with ([f & maps]). The last regs is convenient for sequence processing such as ->>.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, that is a problem with the order of multi-arity function. Umm, if so, it may be better to provide merge-regions-with function ...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! That's a good idea! I split the function into merge-regions-with and merge-regions.


(defn complement-regions
"Returns a sequence of regions complement to in-regions.
in-regoins must be sorted.
Copy link
Member

Choose a reason for hiding this comment

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

typo of in-regions

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

@alumi alumi force-pushed the feature/region-manipulations branch from f1b6447 to cbe4dd7 Compare September 11, 2017 08:54
@totakke totakke merged commit 2899180 into master Sep 11, 2017
@totakke totakke deleted the feature/region-manipulations branch September 11, 2017 09:10
@totakke
Copy link
Member

totakke commented Sep 11, 2017

Thanks :)

@alumi
Copy link
Member Author

alumi commented Sep 11, 2017

Thank you for the helpful advice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants