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

Integration with DataAPI.jl #261

Closed
bkamins opened this issue Jul 26, 2019 · 3 comments · Fixed by JuliaData/DataAPI.jl#7
Closed

Integration with DataAPI.jl #261

bkamins opened this issue Jul 26, 2019 · 3 comments · Fixed by JuliaData/DataAPI.jl#7

Comments

@bkamins
Copy link
Member

bkamins commented Jul 26, 2019

Following the discussion in JuliaData/DataFrames.jl#393 it would be great if you decided to move Between and All definitions to DataAPI.jl.

Also it would help if Not definition were inherited from https://github.com/mbauman/InvertedIndices.jl as other packages currently rely on this. Then probably Not(idxs...) = Not(Any(idxs)) definition should live in DataAPI.jl.

@piever - if you agree to make such a move then we can synchronize with this in DataFrames.jl.

@piever
Copy link
Collaborator

piever commented Jul 27, 2019

Sure, happy to update here as soon as things exist in DataAPI (provided everybody else is also onboard).

Then probably Not(idxs...) = Not(Any(idxs)) definition should live in DataAPI.jl.

Do you mean Not(idxs...) = Not(All(idxs)) (Not(Any(idxs)) reads better though)? I'm mildly concerned about type piracy as Not belongs to InvertedIndices. Maybe easier to add something like Not(args...) = Not(args) in InvertedIndices, or just ask users to do Not(idxs) passing a tuple or Not(All(idxs...)).

I wonder whether most of the logic here https://github.com/JuliaComputing/IndexedTables.jl/blob/master/src/columns.jl#L216 could actually be moved to Tables for an arbitrary Schema (pass schema and selectors, get list of column indices or names).

@bkamins
Copy link
Member Author

bkamins commented Jul 27, 2019

With Not(idxs...) I meant the definition that lives in IndexedTables.jl currently (which would do even more type piracy if it were left there - from 2 packages 😄).

Actually if you are willing to sacrifice Not(idxs...) = Not(All(idxs)) definition I think the things would be simplest. Then, as you write users would just have to add All if they are passing multiple indices (this was my initial idea, but it is breaking for IndextedTables.jl so it is your call). Personally I do not see requiring to add All a big problem.

@mbauman - what do you think about it regarding InvertedIndices.jl changes?

@piever
Copy link
Collaborator

piever commented Jul 27, 2019

Actually if you are willing to sacrifice Not(idxs...) = Not(All(idxs)) definition I think the things would be simplest.

I agree this is best. Not(i, j, k) seems to be used for multidimensional arrays, conflating the two things seems very confusing. I don't remember using Not(args...) with more than one argument in practice. It will just take a little bit longer as we should add a deprecation cycle before switching to using InvertedArrays (I'd add deprecation, keep for one release, then remove deprecation and import InvertedArrays.Not).

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 a pull request may close this issue.

2 participants