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

Already on GitHub? Sign in to your account

Jump to bottom

Fix options of cljam.algo.pileup/pileup #146

Merged
merged 2 commits into from Sep 3, 2018
Merged

Fix options of cljam.algo.pileup/pileup #146

merged 2 commits into from Sep 3, 2018

Conversation

alumi
Copy link
Member

@alumi alumi commented Aug 31, 2018 •
edited

Summary

  • Add an ignore-overlaps? option to cljam.algo.pileup/pileup
  • Skip filtering bases when min-base-quality is zero not positive

Tests

  • lein check 馃啑
  • lein test :all 馃啑

@alumi alumi requested review from athos, totakke and r6eve August 31, 2018 06:54
@codecov
Copy link

codecov bot commented Aug 31, 2018 •
edited

Codecov Report

Merging #146 into master will increase coverage by 0.01%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
+ Coverage   85.72%   85.73%   +0.01%     
==========================================
  Files          69       69              
  Lines        4826     4831       +5     
  Branches      469      469              
==========================================
+ Hits         4137     4142       +5     
  Misses        220      220              
  Partials      469      469
Impacted Files Coverage 螖
src/cljam/algo/pileup.clj 95.89% <88.88%> (+0.14%) 猬嗭笍

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 8abbeb9...c15393a. Read the comment docs.

athos
athos approved these changes Aug 31, 2018
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.

LGTM 馃憤

@alumi
Copy link
Member Author

alumi commented Aug 31, 2018

Added a minor change and amended the commit.

@codecov
Copy link

codecov bot commented Aug 31, 2018 •
edited

Codecov Report

Merging #146 into master will increase coverage by 0.27%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
+ Coverage   85.72%   85.99%   +0.27%     
==========================================
  Files          69       70       +1     
  Lines        4826     5020     +194     
  Branches      469      472       +3     
==========================================
+ Hits         4137     4317     +180     
- Misses        220      231      +11     
- Partials      469      472       +3
Impacted Files Coverage 螖
src/cljam/algo/pileup.clj 95.89% <88.88%> (+0.14%) 猬嗭笍
src/cljam/io/util.clj 100% <0%> (酶) 猬嗭笍
src/cljam/io/wig.clj 90% <0%> (酶)

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 8abbeb9...638dbaa. Read the comment docs.

r6eve
r6eve approved these changes Sep 3, 2018
View reviewed changes
Copy link
Contributor

@r6eve r6eve left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you for fixing that.

@alumi alumi unassigned athos and r6eve Sep 3, 2018
totakke
totakke reviewed Sep 3, 2018
View reviewed changes
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! The code body looks fine, but the descriptions of options are missing in mpileup and create-mpileup. I think you should add the same as pileup description or statements referring to pileup docstring.

src/cljam/algo/pileup.clj Outdated
@@ -187,8 +196,8 @@

(defn mpileup
"Pile up alignments from multiple sources."
[region & sam-readers]
(apply align-pileup-seqs (map #(pileup % region) sam-readers)))
[region options & sam-readers]
Copy link
Member

Choose a reason for hiding this comment

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

A description of options is missing in docstring.

src/cljam/algo/pileup.clj
@@ -197,6 +206,8 @@
([in-sam in-ref out-mplp]
(create-mpileup in-sam in-ref out-mplp nil))
([in-sam in-ref out-mplp region]
(create-mpileup in-sam in-ref out-mplp region nil))
([in-sam in-ref out-mplp region options]
Copy link
Member

Choose a reason for hiding this comment

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

A description of options is missing in docstring.

@alumi
Copy link
Member Author

alumi commented Sep 3, 2018

@totakke Thanks for taking a look! I've added some docstrings.

totakke
totakke approved these changes Sep 3, 2018
View reviewed changes
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.

馃憤

@totakke totakke merged commit 02addc4 into master Sep 3, 2018
@totakke totakke deleted the fix/pileup-options branch September 3, 2018 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@athos athos athos approved these changes

@totakke totakke totakke approved these changes

@r6eve r6eve r6eve approved these changes

Assignees

@totakke totakke

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alumi @athos @totakke @r6eve