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

feat: speed up 2bit file extract #1426

Merged
merged 1 commit into from Mar 14, 2017
Merged

Conversation

@Blaok
Copy link
Contributor

Blaok commented Mar 7, 2017

Rewrite the extract logic.
On a single node with 20 workers, BaseRecalibration time is reduced from 5.8 min to 1.7 min.
Pass existing tests.

On a single node with 20 workers, BaseRecalibration time is reduced from 5.8 min to 1.7 min.
@fnothaft
Copy link
Member

fnothaft commented Mar 7, 2017

Wonderful! Thank you for submitting the patch; I will take a look at this later today or tomorrow.

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 7, 2017

Can one of the admins verify this patch?

@fnothaft
Copy link
Member

fnothaft commented Mar 7, 2017

Jenkins, test this please.

@coveralls
Copy link

coveralls commented Mar 7, 2017

Coverage Status

Coverage decreased (-0.003%) to 76.396% when pulling 1a53ce5 on Blaok:master into 07c1982 on bigdatagenomics:master.

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 7, 2017

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1844/
Test PASSed.

@heuermh
Copy link
Member

heuermh commented Mar 8, 2017

Is there an ε we can provide to coveralls to prevent it from failing a build in silly cases like this one? Coverage decreased by 0.003%? Come on.

Copy link
Member

fnothaft left a comment

LGTM! Thanks for the change!

@fnothaft
Copy link
Member

fnothaft commented Mar 9, 2017

@laserson would you be able to review this today or tomorrow, since you wrote the original 2bit code? If not, I think this is good to go and I will merge as is.

@heuermh
heuermh approved these changes Mar 9, 2017
@fnothaft fnothaft merged commit 1eed8e8 into bigdatagenomics:master Mar 14, 2017
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.003%) to 76.396%
Details
default Merged build finished.
Details
@fnothaft
Copy link
Member

fnothaft commented Mar 14, 2017

Merged! Thanks @Blaok for the contribution! We greatly appreciate it.

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.