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: Upgrade to Gosling v0.9.31 #137

Merged
merged 3 commits into from
Jun 12, 2023
Merged

feat: Upgrade to Gosling v0.9.31 #137

merged 3 commits into from
Jun 12, 2023

Conversation

manzt
Copy link
Member

@manzt manzt commented May 30, 2023

Changes:

  • Bumps Gosling.js to v0.9.31
  • adds gos.bed as a top-level API

TODO:

Need to test out some BED examples. I'm noticing that BED only seems to work for indexed BED files? If BED file isn't indexed, should users prefer the csv loader? @sehilyi @etowahadams

@manzt manzt changed the title feat: Upgrade to Gosling v0.9.30 feat: Upgrade to Gosling v0.9.31 Jun 2, 2023
@manzt manzt requested a review from sehilyi June 2, 2023 16:38
@manzt manzt merged commit f9898ca into main Jun 12, 2023
@manzt manzt deleted the manzt/gosling-v0.9.30 branch June 12, 2023 16:09
@etowahadams
Copy link

I'm noticing that BED only seems to work for indexed BED files? If BED file isn't indexed, should users prefer the csv loader?

Similar discussion in #764 (comment) with respect to VCF and GFF files. It seems like the tentative decision was to require indexing so I followed suit for BED files.

I think it would be nice to also support unindexed files through the BED fetcher API, if the file is small. Currently the only way is through the CSV data fetcher. Is this something we would want to prioritize?

@sehilyi
Copy link
Member

sehilyi commented Jun 26, 2023

I agree that it would be good to support unindexed BED files as well using the bed type data specification. But, given that it is still possible to load them through the CSV data fetcher, I think it is a lower prioritization than implementing other currently unsupported data fetchers.

@manzt
Copy link
Member Author

manzt commented Jun 28, 2023

But, given that it is still possible to load them through the CSV data fetcher, I think it is a lower prioritization than implementing other currently unsupported data fetchers.

I agree with the focus on priorities, but I'm concerned that mandating indexing for BED files might confuse/frustrate end users (especially since I don't see BED support documented in Gosling.js). If I had a .bed file, I'd expect to use gos.bed (or type="bed") to load my data. Knowing that CSV works requires an understanding of the underlying file format. It's important to note that we support a variant of BED that isn't formally described in the spec: BED + index.

For a large number of features, I agree that indexing should be preferred. But requiring tabix-ing files that are totally reasonable to load into memory in order to use designated data loader is a lot of friction.

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.

3 participants