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

Support both ID and name indexing in GFF3Tabix #1069

Closed
wants to merge 1 commit into
base: dev
from

Conversation

Projects
None yet
3 participants
@billzt
Copy link
Contributor

billzt commented Jun 13, 2018

In the current release, when indexing names in GFF3Tabix using generate-names.pl, if a record has both ID and Name attribute, it would only index Name, but not ID. This is very strange.

This commit would support both ID and name indexing.

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Jun 14, 2018

I know this sounds a little pedantic but it would actually be ideal to enable arbitrary fields to be indexed. The --nameAttributes of flatfile-to-json (not generate-names.pl) allows you to choose which fields to be indexed by generate-names.pl

It would be cool to get that full functionality available

@rbuels

This comment has been minimized.

Copy link
Collaborator

rbuels commented Jun 20, 2018

Well, some GFF3 files have just UUIDs or serial numbers for ID, and some have meaningful IDs. Name, on the other hand, is pretty much always meaningful.

The biggest downside I can see to merging this would be bloating the name index with meaningless IDs, for people with meaningless ones. That's probably not a huge concern though, the name index is pretty scalable.

@cmdcolin can you see any other reason not to merge this? If not, do you think you could make a new pull request out of this that also updates the Perl tests to pass, and adds a note in release-notes, close this pull request when you do that, and reference this in the new pull request ?

@rbuels rbuels added this to the 1.15.0 milestone Jun 30, 2018

@rbuels

This comment has been minimized.

Copy link
Collaborator

rbuels commented Jul 5, 2018

@cmdcolin could you add the tests and make a new pull req?

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Jul 11, 2018

Now #1110, close this issue for now

@cmdcolin cmdcolin closed this Jul 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.