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 bgzip indexed fasta support and use indexedfasta npm module #1200

Merged
merged 23 commits into from Sep 13, 2018

Conversation

cmdcolin
Copy link
Contributor

@cmdcolin cmdcolin commented Sep 4, 2018

  • Enables store class deduction from setting only urlTemplate=test.fa.gz in config
  • Enables opening the three files, named test.fa.gz, test.fa.gz.fai, and test.fa.gz.gzi in the Open sequence dialog in browser and electron
  • Uses the @gmod/indexedfasta module (which incorporates 1.2.1 version of @gmod/bgzf-filehandle) which goes through a similar barrage of tests that @gmod/twobit does
  • Produces identical output on volvox sample track tested by just manual inspection and via tests in the indexedfasta module
  • Indexed fasta module has checks for the start >= end returning empty, clamping on start and end of chromosome for coordinates

@ghost ghost assigned cmdcolin Sep 4, 2018
@ghost ghost added the in progress currently being worked on label Sep 4, 2018
@cmdcolin
Copy link
Contributor Author

cmdcolin commented Sep 4, 2018

Note that I did not yet replace the unindexed fasta functionality in core jbrowse with the simple unindexed parser in the indexedfasta module. My aim would be to make the indexedfasta module to generate an index on demand for an unindexed fasta GMOD/indexedfasta-js#2

@cmdcolin cmdcolin added this to the 1.15.4 milestone Sep 4, 2018
Copy link
Contributor

@garrettjstevens garrettjstevens left a comment

Choose a reason for hiding this comment

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

Hey @cmdcolin, PR is looking mostly good to me, but I did have one question. Should we add the new driver you created to the list of drivers below? I wasn't able to get a fa/fai/gzi track to load via dialog box in the browser, but I think adding it to there would fix it. (Dialog box in electron worked just fine.)

this._fileTypeDrivers = [
new BAMDriver(),
new CRAMDriver(),
new BigWigDriver(),
new GFF3Driver(),
new GTFDriver(),
new VCFTabixDriver(),
new VCFTribbleDriver(),
new BEDTabixDriver(),
new GFF3TabixDriver(),
new BEDDriver(),
new BigBedDriver()
];

@garrettjstevens
Copy link
Contributor

Actually, come to think of it, should the IndexedFastaDriver be in that list, too?

@cmdcolin
Copy link
Contributor Author

cmdcolin commented Sep 6, 2018

@garrettjstevens there is a separate dialog for loading files src/JBrowse/View/FastaFileDialog.js

would be interested to know why it isn't working though

@cmdcolin
Copy link
Contributor Author

cmdcolin commented Sep 6, 2018

actually, note that you would need to use Open sequence and not Open track to get it to work. It's kind of annoying that Open track and Open sequence are so similar, and that it is sort of a silent "failure" if you use Open track instead of Open sequence. let me know if that helps though

@garrettjstevens garrettjstevens dismissed their stale review September 6, 2018 16:09

Was looking at the wrong dialog box

Copy link
Contributor

@garrettjstevens garrettjstevens left a comment

Choose a reason for hiding this comment

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

Ah shoot, I was looking at Open File instead of Open Sequence. My bad. Maybe in JBrowse2 we can architect things so that distinction is a bit more clear. PR looks good to me.

@cmdcolin
Copy link
Contributor Author

I guess if theres no qualms I might go ahead and merge soon. I also noted this feedback from a user about jbrowse desktop and created a new issue #1205 but should not necessarily block this PR

@cmdcolin cmdcolin merged commit b793bbd into dev Sep 13, 2018
@ghost ghost removed the in progress currently being worked on label Sep 13, 2018
@cmdcolin cmdcolin deleted the fasta_npm_module branch September 22, 2018 00:15
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

2 participants