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

add_edge!(g, u, v) deletes existing edges if called twice with the same arguments #70

Closed
degregat opened this issue Oct 19, 2023 · 4 comments · Fixed by #71
Closed

add_edge!(g, u, v) deletes existing edges if called twice with the same arguments #70

degregat opened this issue Oct 19, 2023 · 4 comments · Fixed by #71
Labels
bug Something isn't working

Comments

@degregat
Copy link

degregat commented Oct 19, 2023

add_edge!(g, u, v) from here, when called with the same u, v twice, adds the edge the first time, deletes it the second time (and does not add it in subsequent calls), since the length of g.graph does not change.

Example

using Graphs
using MetaGraphsNext

dg = MetaGraph(
    DiGraph();
    label_type = String,
    vertex_data_type = String,
)

add_vertex!(dg, "u", "foo")
add_vertex!(dg, "v", "bar")
add_edge!(dg, "u", "v")
println(length(dg.edge_data)) # == 1
add_edge!(dg, "u", "v")
println(length(dg.edge_data)) # == 0

g = MetaGraph(
    Graph();
    label_type = String,
    vertex_data_type = String,
)

add_vertex!(g, "u", "foo")
add_vertex!(g, "v", "bar")
add_edge!(g, "u", "v")
println(length(dg.edge_data)) # == 1 
add_edge!(g, "u", "v")
println(length(dg.edge_data)) # == 0

Workaround

# For directed graphs
if !haskey(g.edge_data, (u, v))
  add_edge!(g, u, v)
end

# For undirected graphs
if !haskey(g.edge_data, (u, v)) && !haskey(g.edge_data, (v, u))
  add_edge!(g, u, v)
end

Proposed Fix

Remove this call of delete! and only return false. If that does not lead to the expected behavior, add checks as described in the workaround inside add_edge!, s.t. users don't have to do that.

@gdalle
Copy link
Member

gdalle commented Oct 19, 2023

Thanks for the issue! It seems like a problem indeed, do you want to open a PR?

@degregat
Copy link
Author

degregat commented Oct 19, 2023

I can do that 😄

Do you think removing delete! will have unintended consequences, e.g. what's the worst that can happen if add_edge!() (from Graphs.jl) is called repeatedly with the same codes it should resolve to from the labels?

@bramtayl
Copy link
Collaborator

I think the only reason the delete! was there was in case adding the edge failed for another reason, e.g. the graph ran out of room or something. In which case, we should only delete if the edge doesn't exist. So we could change this line to !has_edge(meta_graph.graph, code_1, code_2)?

@cecileane
Copy link
Contributor

I discovered that too, and will add a fix to a PR very soon.

cecileane added a commit to cecileane/MetaGraphsNext.jl that referenced this issue Oct 21, 2023
@gdalle gdalle added the bug Something isn't working label Oct 22, 2023
@gdalle gdalle linked a pull request Oct 22, 2023 that will close this issue
gdalle added a commit that referenced this issue Oct 26, 2023
* fix #69 and #70

* v0.7.0 because breaking change

* arrange: no more codes as arguments

* more testing

* add_edge! modifies edge data if edge already present

* Update src/graphs.jl

---------

Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com>
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

Successfully merging a pull request may close this issue.

4 participants