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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create an index file while writing a sorted BAM file #197

Merged
merged 1 commit into from
Jun 18, 2020
Merged

Create an index file while writing a sorted BAM file #197

merged 1 commit into from
Jun 18, 2020

Conversation

xckitahara
Copy link
Contributor

This PR is to add a function to create an index file if it is already sorted when creating a BAM file.
PR for issue #141 .

@codecov
Copy link

codecov bot commented May 29, 2020 •
edited
Loading

Codecov Report

Merging #197 into master will increase coverage by 0.27%.
The diff coverage is 86.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
+ Coverage   86.91%   87.18%   +0.27%     
==========================================
  Files          77       77              
  Lines        6267     6362      +95     
  Branches      520      516       -4     
==========================================
+ Hits         5447     5547     +100     
- Misses        300      301       +1     
+ Partials      520      514       -6
Impacted Files Coverage Δ
src/cljam/io/bam_index/writer.clj 84.87% <73.23%> (+0.50%) ⬆️
src/cljam/io/bam/core.clj 97.05% <88.88%> (-2.95%) ⬇️
src/cljam/tools/cli.clj 59.74% <90.00%> (+0.70%) ⬆️
src/cljam/io/bam/writer.clj 92.77% <98.33%> (+27.05%) ⬆️
src/cljam/algo/convert.clj 90.00% <100.00%> (ø)
src/cljam/io/sam.clj 100.00% <100.00%> (ø)
src/cljam/io/util/bin.clj 97.26% <0.00%> (+2.92%) ⬆️

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 84a535d...778879a. Read the comment docs.

@xckitahara xckitahara changed the title Feature/create index while writing bam Create an index file while writing a sorted BAM file May 29, 2020
@xckitahara xckitahara force-pushed the feature/create-index-while-writing-bam branch from 0530bdb to cf845bf Compare June 3, 2020 07:13
@xckitahara xckitahara marked this pull request as ready for review June 8, 2020 07:16
@xckitahara xckitahara requested review from alumi and a team as code owners June 8, 2020 07:16
@xckitahara xckitahara requested review from niyarin and removed request for a team June 8, 2020 07:16
@alumi
Copy link
Member

alumi commented Jun 8, 2020

Hi @xckitahara, Thank you for working on this feature!
Could you squash your commits into some concise and meaningful ones and then rebase them onto the master branch?

@xckitahara xckitahara force-pushed the feature/create-index-while-writing-bam branch 2 times, most recently from 65c0569 to 7f5a7b6 Compare June 8, 2020 09:08
@alumi
Copy link
Member

alumi commented Jun 8, 2020

It seems like GitHub cannot associate your commits with your account @xckitahara.
Please configure your git client with git config --global user.email.

@xckitahara
Copy link
Contributor Author

Hi @xckitahara, Thank you for working on this feature!
Could you squash your commits into some concise and meaningful ones and then rebase them onto the master branch?

@alumi Has been updated.

@xckitahara
Copy link
Contributor Author

It seems like GitHub cannot associate your commits with your account @xckitahara.
Please configure your git client with git config --global user.email.

I'm sorry.
Set up.

@alumi
Copy link
Member

alumi commented Jun 8, 2020 •
edited
Loading

Set up

Great 👍
Now you need to amend —reset-author commits to fix the committer and the author

alumi
alumi reviewed Jun 9, 2020
View reviewed changes
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.

I’m sorry I’m only half way through but please let me submit my review comments

src/cljam/io/sam.clj Outdated Show resolved Hide resolved
src/cljam/io/sam.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/core.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
@xckitahara xckitahara force-pushed the feature/create-index-while-writing-bam branch from 7f5a7b6 to e2b156c Compare June 9, 2020 00:48
alumi
alumi reviewed Jun 9, 2020
View reviewed changes
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
src/cljam/io/bam_index/writer.clj Outdated Show resolved Hide resolved
alumi
alumi reviewed Jun 9, 2020
View reviewed changes
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
src/cljam/io/bam_index/writer.clj Outdated Show resolved Hide resolved
test/cljam/io/sam_test.clj Outdated Show resolved Hide resolved
test/cljam/io/sam_test.clj Show resolved Hide resolved
src/cljam/io/bam_index/writer.clj Outdated Show resolved Hide resolved
alumi
alumi reviewed Jun 9, 2020
View reviewed changes
src/cljam/io/bam/writer.clj Outdated Show resolved Hide resolved
alumi
alumi approved these changes Jun 12, 2020
View reviewed changes
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 👍

@xckitahara
Copy link
Contributor Author

Performance comparison

Time measurement of sort:
① before change,
② after change (with bai created),
③ after change (without bai created)
It was measured at. (Since sort is used for both aln write and block write)
The target file is medium-sam-file used in the test etc.

① time: 761.079912 ms, sd: 98.673396 µs
② time: 925.187657 ms, sd: 47.099024 µs
③ time: 741.354834 ms, sd: 76.992721 µs

niyarin
niyarin approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@niyarin niyarin left a comment

Choose a reason for hiding this comment

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

LGTM:+1:

@xckitahara xckitahara force-pushed the feature/create-index-while-writing-bam branch from 20e6b08 to 778879a Compare June 18, 2020 02:29
@alumi alumi merged commit b0d1400 into master Jun 18, 2020
@alumi alumi deleted the feature/create-index-while-writing-bam branch June 18, 2020 02:44
@alumi alumi mentioned this pull request Jun 18, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niyarin niyarin niyarin approved these changes

@alumi alumi alumi approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xckitahara @alumi @niyarin