PartitionedGraphs extension for DataGraphs and interface overhaul.
#55
Conversation
…btype `AbstractNamedGraph` - Interface now no longer assumes vertex and edge date stored as mutable field. Instead, requires overloading setters and getters. - Subtyping `AbstractNamedGraph` cuts down on the method forwarding almost entirely.
…rams of abstract type.
…Graph` Tests have been adjusted accordingly.
… `getindex` This now correctly mirrors `DataGraphs` interface.
This is in anticipation of making this change in `DataGraphs`
These are defined on the abstract type
jack-dunham
force-pushed
the
partitions
branch
from
89a155a to
be9c19a
Compare
jack-dunham
changed the title
PartitionedGraphs extension for DataGraphs.PartitionedGraphs extension for DataGraphs and interface overhaul.
|
(I closed and reopened the PR to trigger the tests since NamedGraphs.jl v0.9 is now registered.) |
|
@jack-dunham it looks like |
This is to be enabled once `edges_subgraph` is defined for data graphs.
…`edge_data_eltype`
This will update the underlying graph.
src/dataview.jl
Outdated
| function Base.copyto!(dest::VertexDataView, bc::Base.Broadcast.Broadcasted) | ||
| for (i, vertex) in enumerate(vertices(dest.graph)) | ||
| dest[vertex] = bc[i] | ||
| end | ||
| return dest | ||
| end | ||
|
|
||
| function Base.copyto!(dest::EdgeDataView, bc::Base.Broadcast.Broadcasted) | ||
| for (i, edge) in enumerate(edges(dest.graph)) | ||
| dest[edge] = bc[i] | ||
| end | ||
| return dest | ||
| end |
There was a problem hiding this comment.
I think these definitions aren't quite right. I think the issue is that the ordering of the data on the vertices and edges are an implementation detail, so if you do:
g = DataGraph(["x", "y"])
# Or:
g = DataGraph(["y", "x"])
VertexDataView(g) .= [1, 2]the results will be different from each other, i.e. the result is sensitive to the vertex ordering.
I think we should follow the example of Dictionaries.jl and only support the following:
VertexDataView(g) .= 1 # Ok since the ordering doesn't matter
VertexDataView(g) .= Dictionary(["x", "y"], [1, 2]) # Specify the ordering
VertexDataView(g)[Vertices(["x", "y"])] .= [1, 2] # Alternative to the abovewhich is comparable to the behavior of Dictionaries.jl:
julia> d = Dictionary(["x", "y"], [1, 2])
2-element Dictionary{String, Int64}:
"x" │ 1
"y" │ 2
julia> d .= 3
2-element Dictionary{String, Int64}:
"x" │ 3
"y" │ 3
julia> d .= Dictionary(["x", "y"], [4, 5])
2-element Dictionary{String, Int64}:
"x" │ 4
"y" │ 5
julia> d .= [4, 5] # ErrorsThere was a problem hiding this comment.
Something slightly unfortunate about Dictionaries.jl broadcasting is that it doesn't support cases where the indices aren't ordered in the same way or where the indices being set are a subset of the dictionary that is being assigned:
julia> d = Dictionary(["x", "y"], [1, 2])
2-element Dictionary{String, Int64}:
"x" │ 1
"y" │ 2
julia> d .= Dictionary(["y", "x"], [4, 3])
ERROR: IndexError("Indices do not match")
julia> d .= Dictionary(["x"], [3])
ERROR: IndexError("Indices do not match")so maybe we can mostly use the Dictionaries.jl broadcasting implementation (https://github.com/andyferris/Dictionaries.jl/blob/master/src/broadcast.jl) but then have a custom overload of Base.copyto!(out::VertexDataView, d::BroadcastedDictionary) that allows for those two cases.
|
|
||
| # ======================== DataGraphs interface for QuotientView ========================= # | ||
|
|
||
| function NamedGraphs.get_graph_index(qv::QuotientView{V, <:AbstractDataGraph}, ind) where {V} |
There was a problem hiding this comment.
| function NamedGraphs.get_graph_index(qv::QuotientView{V, <:AbstractDataGraph}, ind) where {V} | |
| function NamedGraphs.get_graph_index(qv::QuotientView{<:Any, <:AbstractDataGraph}, ind) |
Just a style thing since I don't see V used.
|
Seems tests are breaking; I'll check. |
2 participants
This PR introduces a change to the
DataGraphsinterface. The following functions must now be overloaded for a graph to behave like a data graph:The functions
getindex,setindex!get!etc are all defined in terms of those. It is a bit more verbose than just having a functionvertex_dataetc that returns a mutable dictionary, but I think it is ultimately more generic. The return value ofvertex_dataetc is now constructed on the fly, so doingsetindex!(vertex_data(g), val, vertex)will not modify the underlying graph.The benefit of this change is that one no longer require the data on the vertices and edges to be stored as a mutable field; it can be generated on the fly. This then results in constant behavior when defining views of vertex/edge data. One can still use a mutable dictionary to store the data for a given concrete
AbstractDataGraphsubtype.Other Changes
AbstractDataGraphnow subtypesAbstractNamedGraph. This greatly reduces required method overloads.VertexDataViewandEdgeDataViewobjects that behave like dictionaries with respect to the graph data.PartitionedGraphspackage.