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

Tables.subset problems #56

Open
slwu89 opened this issue Sep 22, 2023 · 2 comments
Open

Tables.subset problems #56

slwu89 opened this issue Sep 22, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@slwu89
Copy link
Member

slwu89 commented Sep 22, 2023

Based on the "Dendrograms" section of the tests, and a cursory look at the source for subset, it seems that the following should work, but currently error:

Tables.subset(td.X,1) # works
Tables.subset(td.X,2)

# do not work
Tables.subset(td.X,1:2)
Tables.subset(td.X,[1,2])
Tables.subset(td.X,:)

I plan to look into this soon.

@epatters epatters added the bug Something isn't working label Sep 22, 2023
@epatters
Copy link
Member

Thanks for the report. I agree that we should support the Tables.jl interface where possible.

@slwu89
Copy link
Member Author

slwu89 commented Sep 22, 2023

@epatters not sure if this solution is too janky. The issue is that we need Tables.schema to work on objects of type ACSetTable. Anyway, this works.

using ACSets
using Tables

SchDendrogram = BasicSchema([:X], [(:parent,:X,:X)], [:R], [(:height,:X,:R)])

@abstract_acset_type AbstractDendrogram
@acset_type Dendrogram(SchDendrogram, index=[:parent]) <: AbstractDendrogram

d = Dendrogram{Int}()

add_parts!(d, :X, 3, height=0)
add_parts!(d, :R, 2)
add_parts!(d, :X, 2, height=[10,AttrVar(add_part!(d,:R))])
set_subpart!(d, 1:3, :parent, 4)
set_subpart!(d, [4,5], :parent, 5)

td = tables(d)
Tables.subset(td.X,1)
Tables.subset(td.X,2)
Tables.subset(td.X,1:2)

# need to be able to get a Tables.Schema out of an ACSetTable obj
import Tables: schema
function Tables.schema(tab::ACSets.DenseACSets.ACSetTable)
    names = Tables.columnnames(tab)
    Tables.Schema(names, [typeof(getindex(tab,name)) for name in names])
end
Tables.subset(td.X,1:2)
Tables.subset(td.X,[1,2])
Tables.subset(td.X,:)

[EDIT] Actually this might be a subpar solution, as now Tables.subset returns a different data type than an ACSetTable (returns a Tables.DictColumnTable, see https://github.com/JuliaData/Tables.jl/blob/25be9d246c2d9e0f31f694476164631d3e4f7b52/src/Tables.jl#L608). I'm guessing the spirit of ACSets.jl would be instead to extend the method Tables.subset directly. What do you think? Happy to get a PR doing that. In that case, do we also still want to extend Tables.schema as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants