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

Add failing test case and fix for incorrect chunk merges #36

Merged
merged 1 commit into from
Aug 23, 2019

Conversation

cmdcolin
Copy link
Collaborator

It can be seen in 1.0.20 and 1.0.21 that there are breaks in long nanopore features

localhost_jbrowse__data=hg19 loc=chr22%3A16564260 16565959 tracks=nanopore highlight= (1)

The "completely resolved chunk" merging caused this. The test case here demonstrates it albeit with a sort of large test file of ~14mb

Potentially we need to review yet the other chunk merging case

@codecov
Copy link

codecov bot commented Aug 22, 2019

Codecov Report

Merging #36 into master will decrease coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
- Coverage   81.49%   81.35%   -0.14%     
==========================================
  Files          12       12              
  Lines         935      928       -7     
  Branches      196      195       -1     
==========================================
- Hits          762      755       -7     
  Misses        148      148              
  Partials       25       25
Impacted Files Coverage Δ
src/bai.js 97.12% <ø> (-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 db4ef17...b84c90d. Read the comment docs.

@cmdcolin
Copy link
Collaborator Author

I also wanted to see how this "test" performs in the old version, with chunk merges, and with this version, with the worry that the merging was more efficient. From what I can tell this version is more efficient

Old version 1.5s, unzips ~80 chunks
New version 1s, unzips ~130 chunks

So new code is still modestly faster. It does make one wonder if there is a way to speed up the unziping, but as above, the chunk merging is not apparently a bad thing (tm)

_home_cdiesh_src_gmod_bam-js_9407 0x_flamegraph html (1)

@cmdcolin cmdcolin merged commit 248bb54 into master Aug 23, 2019
@cmdcolin
Copy link
Collaborator Author

Probably same code will need to go into tabix-js

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

Successfully merging this pull request may close these issues.

None yet

1 participant