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

Use bamjs #1215

Merged
merged 21 commits into from Oct 5, 2018

Conversation

Projects
None yet
2 participants
@cmdcolin
Copy link
Contributor

cmdcolin commented Sep 25, 2018

This is a PR to use the @gmod/bam module for parsing BAM data

This has been tested with

  • CSI and BAI files
  • Tests pulled over from jasmine to the npm module
  • Paired end reads from 1000 genomes, RNA seq, volvox data, etc
  • Manual inspection of the the rendered tracks between dev and this branch for different BAM files
  • Manual inspection comparison of the dialog boxes between dev and this branch for different BAM files

Overall, I think the parity between this and the BAM code is quite good. The BAMCombination concept is not implemented just yet but I wanted to make this available for review :)

@wafflebot wafflebot bot added the in progress label Sep 25, 2018

@cmdcolin cmdcolin force-pushed the use_bamjs branch from 63fb80b to c025125 Sep 25, 2018

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Sep 25, 2018

The BAMCombination should have basic support now (it can combine the volvox long reads and volvox-sorted track as union)

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Sep 30, 2018

There is an error when opening up local BAMs with this currently

Also I will need to see if I can address some issues discussed with Rob including possibly adopting the unzip methods from @gmod/tabix (because @gmod/bam and current BAM code in jbrowse always overfetches it needs a way to correctly get the actual block lengths that it is decoding)

@cmdcolin cmdcolin force-pushed the use_bamjs branch from ec177b4 to 4206dff Oct 1, 2018

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Oct 1, 2018

Opening up local BAM files works now :)

Potential concerns with the code include

  1. blowing memory limits due to resolving a Promise.all with large data, but it looks like in principle the cram-js code doesn't do much different?
  2. sometimes the bam-js code has to do some awkward work around file reads because it fetches an extra block size each time (this is coded into current BAM code too) so since that could be EOF it always has to ensure it's getting enough bytesRead, but at this point it seems like it's doing this fine

Anyways, I am open to review on this! I think it's at a good stage unless those two issues need any specific attention

@cmdcolin cmdcolin force-pushed the use_bamjs branch 2 times, most recently from 964fdfe to 7ab7b4b Oct 2, 2018

@rbuels

This comment has been minimized.

Copy link
Collaborator

rbuels commented Oct 3, 2018

Per dev meeting conversation on 10/3, will wait to merge this until after 1.15.4 is out

@cmdcolin cmdcolin force-pushed the use_bamjs branch from 7ab7b4b to 1201c63 Oct 5, 2018

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Oct 5, 2018

Maybe we can make this mainline now post 1.15.4 release :)

@cmdcolin cmdcolin merged commit 50320e6 into dev Oct 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Oct 5, 2018

@cmdcolin cmdcolin deleted the use_bamjs branch Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment