ITensor / DataGraphs.jl

A simple graph type with data on the vertices and edges.
MIT License
12 stars 3 forks source link

Fix `union` precedence #22

Open mtfishman opened 4 months ago

mtfishman commented 4 months ago

The precedence of data overwriting in union of DataGraphs is inconsistent with union for other types:

julia> using DataGraphs

julia> g1 = DataGraph(4);

julia> g2 = DataGraph(4);

julia> g1[1 => 2] = 1;

julia> g2[1 => 2] = 2;

julia> union(g1, g2)
DataGraph{Int64, Any, Any, Graphs.SimpleGraphs.SimpleGraph{Int64}, Graphs.SimpleGraphs.SimpleEdge{Int64}} with 4 vertices:
Base.OneTo(4)

and 0 edge(s):

with vertex data:
0-element Dictionaries.Dictionary{Int64, Any}

and edge data:
1-element Dictionaries.Dictionary{Graphs.SimpleGraphs.SimpleEdge{Int64}, Any}
 Edge 1 => 2 │ 2

compared to:

julia> union(Any[1], Any[1.0])
1-element Vector{Any}:
 1
mtfishman commented 4 months ago

Comes up in https://github.com/mtfishman/ITensorNetworks.jl/pull/141.

b-kloss commented 4 months ago

I would expect union to perform a union on the edge_data for edges in both g1 and g2. A more appropriate name for this function may be merge, which would also reveal that argument order matters (which I found counterintuitive for union mathematically speaking, but is also relevant for Base.union).

mtfishman commented 4 months ago

I could see an argument for giving it a different name (I think the closest function in modern Julia would be mergewith). The reason for calling it union is that it is a union of the underlying graphs. Then you can control how the data is merged with extra keyword arguments: https://github.com/mtfishman/DataGraphs.jl/blob/v0.1.12/src/abstractdatagraph.jl#L192-L204

mtfishman commented 4 months ago

Basically, the current definition follows more closely to the definition of a graph union as a union of the vertices and edges: https://en.wikipedia.org/wiki/Graph_operations#Binary_operations.