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

same signature for Chunks() csi and tabix #44

Closed
brentp opened this issue Nov 5, 2016 · 8 comments
Closed

same signature for Chunks() csi and tabix #44

brentp opened this issue Nov 5, 2016 · 8 comments

Comments

@brentp
Copy link
Contributor

brentp commented Nov 5, 2016

having an external library support both csi and tabix would be simpler if they both had the same signature for Chunks() would you consider changing CSI to signature of

func (i *Index) Chunks(r Record) ([]bgzf.Chunk, error)

to match tabix?

@kortschak
Copy link
Member

I think this is reasonable, but I'm reluctant to keep changing API. If we can sort out what changes need to be made then do them all at once, followed by a stability guarantee, then I'm OK with it.

The Record type would probably want to be in a common package, but I don't see where. Where would you propose it go? One possibility is to uninternalise internal as index and place it there.

@brentp
Copy link
Contributor Author

brentp commented Nov 5, 2016

seems a shame to rename internal just to expose a single interface. But, assuming you dont want to import tabix into csi, I dont see a better alternative.

@brentp
Copy link
Contributor Author

brentp commented Nov 5, 2016

I guess it could go in bgzf

@kortschak
Copy link
Member

I thought about bgzf and it's not the right level of abstraction.

Are there any other API changes you'd like to see?

@brentp
Copy link
Contributor Author

brentp commented Nov 5, 2016

Not that I've noticed.
Anything that would be needed to be moved for cram?

@kortschak
Copy link
Member

I try to avoid thinking about CRAM.

Looking at the signature of tabix and csi Chunks methods, they cannot be merged. One takes an RID integer and the other takes a string. The csi Chunks method could be altered to take a csi.Record, but I don't really see the benefit in that. Can you explain to me what you want out of this?

@kortschak
Copy link
Member

kortschak commented Nov 5, 2016

It seems to me that the change should be in tabix. bam and csi take ref, beg, end, while tabix takes an interface that can give these values. Having it take this interface value in hindsight is odd.

@brentp
Copy link
Contributor Author

brentp commented Nov 5, 2016

I was thinking if I had

type Indexed interface {
    Chunks(???) []bgzf.Chunk
}

then I could support both tabix and csi with little code change after I had the index read in. I'm fine with a change to tabix instead, I just figured a change to csi would affect less external code.

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

No branches or pull requests

2 participants