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

Optimization for SNPCoverageAdapter and CRAM parsing #2947

Merged
merged 10 commits into from
May 10, 2022
Merged

Conversation

cmdcolin
Copy link
Collaborator

This can save about ~1second in generateCoverageBins calls on a 20kb region

Allocates less nested objects

On 200x coverage shortreads from jb2profile

Time spent in generateCoverageBins specifically:

This branch 2.82s
Main branch 3.6s

Possibly we could also bin multiple bp into a single bin (jb1 does this) and or use skips_and_dels (another jb1-ism) to optimize further, but we would need to be careful about binning multiple bp into a single bin with snps/modifications as those should probably not be aggregated

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label May 10, 2022
@cmdcolin cmdcolin added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels May 10, 2022
@cmdcolin cmdcolin marked this pull request as draft May 10, 2022 16:29
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #2947 (9361a15) into main (d14ea8e) will decrease coverage by 0.06%.
The diff coverage is 62.65%.

@@            Coverage Diff             @@
##             main    #2947      +/-   ##
==========================================
- Coverage   60.74%   60.67%   -0.07%     
==========================================
  Files         601      601              
  Lines       27329    27341      +12     
  Branches     6643     6654      +11     
==========================================
- Hits        16600    16590      -10     
- Misses      10419    10441      +22     
  Partials      310      310              
Impacted Files Coverage Δ
...rc/LinearSNPCoverageDisplay/components/Tooltip.tsx 29.62% <0.00%> (ø)
...ments/src/SNPCoverageAdapter/SNPCoverageAdapter.ts 56.37% <50.00%> (-2.81%) ⬇️
...s/alignments/src/PileupRenderer/PileupRenderer.tsx 48.20% <60.86%> (-0.38%) ⬇️
...gnments/src/CramAdapter/CramSlightlyLazyFeature.ts 80.20% <93.33%> (-5.44%) ⬇️
...ckages/core/pluggableElementTypes/RpcMethodType.ts 83.33% <0.00%> (-2.09%) ⬇️
...iew/src/LinearGenomeView/components/CenterLine.tsx 100.00% <0.00%> (+8.33%) ⬆️

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 d14ea8e...9361a15. Read the comment docs.

@cmdcolin
Copy link
Collaborator Author

This PR now also adds some CRAM optimizations too (raw loops instead of foreach, preallocate mismatches). Combined with @gmod/cram optimization, this branch is about ~20% faster than main (the @gmod/cram was about 10%-15% faster) for longread CRAM

400xcram longread
main 39.545
this branch 29.794
1.2x faster

600xcram longread
main 51.299s
this branch 39.545s
1.2x faster

800xcram longread
main 63.608s
this branch 51.496s
1.2x faster

@cmdcolin
Copy link
Collaborator Author

note: these changes above improve the longread performance more while the @gmod/cram pr improve shortread more

@cmdcolin cmdcolin changed the title Optimization for SNPCoverageAdapter Optimization for SNPCoverageAdapter and CRAM parsing May 10, 2022
@cmdcolin cmdcolin marked this pull request as ready for review May 10, 2022 21:33
@cmdcolin cmdcolin merged commit 212a9d4 into main May 10, 2022
@cmdcolin cmdcolin deleted the new_snpcov branch May 10, 2022 22:07
@cmdcolin cmdcolin added enhancement New feature or request and removed bug Something isn't working labels May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant