You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Should graph[label] return the vertex metadata or the integer code? #44
No matter what we do, if we want to use functions from Graphs.jl with labels, it will have to be wordy. I tried to explain it here (#39 (comment)): basically the only alternative would be to reimplement all of Graphs.jl with labels, not juste a few functions like inneighbors
there is still some space to work before thinking of doing such a mass forwarding which I also find demanding. For example, we could tread getindex(::MetaGraph, ::Integer) differently from getindex(::MetaGraph, ::Label). And if we also implemenent getindex(::MetaGraph, <:Vector{Integer}) something like that will work:
colors[inneighbors(colors, colors[:blue])]
so MetaGraph[::Label] outputs an Integer and MetaGraph[::Integer] outputs the data
ofc as I say here this will require to ban users from using Integer Labels, which I am okey with. Something like UUIDs should be encouraged instead of Integers eitherway.
For example, we could tread getindex(::MetaGraph, ::Integer) differently from getindex(::MetaGraph, ::Label)
That's an idea I actually like. The only reasonable use of integer labels is to compensate for vertex code updating following deletions or to provide default identifiers (as seen in #37 for instance). Maybe that's worth giving up on for shorter syntax. @bramtayl thoughts?
I think I'm ready to fully ban integer labels. Then we can use multiple dispatch to make codes and labels interchangeable, which several people have asked for. We might need to add a note to the documentation that using codes can be more performant in some cases, because it avoids having to look up the label multiple times.
Should graph[label] return the vertex metadata or the integer code?
If we make codes and labels interchangable, then hopefully users won't even have to interact with the codes at all. In that case, I think returning the vertex metadata would be more useful?
We could treat getindex(::MetaGraph, ::Integer) differently from getindex(::MetaGraph, ::Label)
That seems maybe a bit confusing: I'd expect them to return the same thing?
Oh, just so that you could do e.g. inneighbors(graph, 1) or inneighbors(graph, :label1)
The thing is, if we do that, there's no reason to stop there, and we might end up re-implementing all of Graphs.jl with labels instead of codes.
I think the getindex dispatch is elegant, functional (if we ban integer labels) and will substantially reduce code size. But it might be confusing so it needs good docs
Activity
filchristou commentedon Feb 22, 2023
and how would you get the vertex metadata ? define a
get_propor something ?gdalle commentedon Feb 22, 2023
Yeah that's always the compromise, if you get one fast you get the other one slowly. I would favor
get_dataif we decide to switch.I have no strong opinion, I just thought it's a matter worth discussing while we stabilize the API
filchristou commentedon Feb 22, 2023
so what previously in #39 was done with
and I wanted it to be like
with your proposition it will end up looking:
which is still pretty wordy.
I mean you will have to keep the
label_for, right ?Are or you having any other thoughts ?
gdalle commentedon Feb 22, 2023
No matter what we do, if we want to use functions from Graphs.jl with labels, it will have to be wordy. I tried to explain it here (#39 (comment)): basically the only alternative would be to reimplement all of Graphs.jl with labels, not juste a few functions like
inneighborsfilchristou commentedon Feb 22, 2023
there is still some space to work before thinking of doing such a mass forwarding which I also find demanding. For example, we could tread
getindex(::MetaGraph, ::Integer)differently fromgetindex(::MetaGraph, ::Label). And if we also implemenentgetindex(::MetaGraph, <:Vector{Integer})something like that will work:so
MetaGraph[::Label]outputs an Integer andMetaGraph[::Integer]outputs the dataofc as I say here this will require to ban users from using Integer Labels, which I am okey with. Something like
UUIDs should be encouraged instead of Integers eitherway.gdalle commentedon Feb 22, 2023
That's an idea I actually like. The only reasonable use of integer labels is to compensate for vertex code updating following deletions or to provide default identifiers (as seen in #37 for instance). Maybe that's worth giving up on for shorter syntax. @bramtayl thoughts?
bramtayl commentedon Feb 22, 2023
I think I'm ready to fully ban integer labels. Then we can use multiple dispatch to make codes and labels interchangeable, which several people have asked for. We might need to add a note to the documentation that using codes can be more performant in some cases, because it avoids having to look up the label multiple times.
If we make codes and labels interchangable, then hopefully users won't even have to interact with the codes at all. In that case, I think returning the vertex metadata would be more useful?
That seems maybe a bit confusing: I'd expect them to return the same thing?
gdalle commentedon Feb 22, 2023
Then what do you mean by using multiple dispatch on codes and labels?
bramtayl commentedon Feb 25, 2023
Oh, just so that you could do e.g.
inneighbors(graph, 1)orinneighbors(graph, :label1)gdalle commentedon Mar 2, 2023
The thing is, if we do that, there's no reason to stop there, and we might end up re-implementing all of Graphs.jl with labels instead of codes.
I think the
getindexdispatch is elegant, functional (if we ban integer labels) and will substantially reduce code size. But it might be confusing so it needs good docs