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

Fix union precedence #22

Open
mtfishman opened this issue Feb 29, 2024 · 4 comments
Open

Fix union precedence #22

mtfishman opened this issue Feb 29, 2024 · 4 comments

Comments

@mtfishman
Copy link
Member

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 => 22

compared to:

julia> union(Any[1], Any[1.0])
1-element Vector{Any}:
 1
@mtfishman
Copy link
Member Author

Comes up in ITensor/ITensorNetworks.jl#141.

@b-kloss
Copy link

b-kloss commented Feb 29, 2024

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
Copy link
Member Author

mtfishman commented Feb 29, 2024

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
Copy link
Member Author

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.

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

No branches or pull requests

2 participants