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

digraph_union fails if merge_edges is set to true #432

Closed
georgios-ts opened this issue Aug 31, 2021 · 2 comments · Fixed by #439
Closed

digraph_union fails if merge_edges is set to true #432

georgios-ts opened this issue Aug 31, 2021 · 2 comments · Fixed by #439
Labels
bug Something isn't working

Comments

@georgios-ts
Copy link
Collaborator

Information

  • retworkx version: main
  • Python version: Any
  • Rust version: Any
  • Operating system: Any

What is the current behavior?

If merge_edges arg is set to True, digraph_union returns wrong output. This happens in many cases (e.g graphs with node holes, mismatch edge weights) and this means we need to rework the logic of the function.

What is the expected behavior?

Fix this.

Steps to reproduce the problem

An example where an edge is deleted when it shouldn't since edge weights don't match:

import retworkx as rx

first = rx.PyDiGraph()
nodes = first.add_nodes_from([0, 1])
first.add_edges_from([
   (nodes[0], nodes[1], 'a')
])

second = rx.PyDiGraph()
nodes = second.add_nodes_from([0, 1])
second.add_edges_from([
    (nodes[0], nodes[1], 'b')
])

final = rx.digraph_union(first, second, 
                         merge_nodes=True,
                         merge_edges=True
)
print(final.weighted_edge_list())
---
WeightedEdgeList[(0, 1, a)]

An example with node index hole where an edge is not deleted but it should be removed.

import retworkx as rx

first = rx.PyDiGraph()
nodes = first.add_nodes_from([0, 1])
first.add_edges_from([
   (nodes[0], nodes[1], 'a')
])

second = rx.PyDiGraph()
dummy = second.add_node('dummy')
nodes = second.add_nodes_from([0, 1])
second.add_edges_from([
    (nodes[0], nodes[1], 'a')
])
second.remove_node(dummy)

final = rx.digraph_union(first, second, 
                         merge_nodes=True,
                         merge_edges=True
)
print(final.weighted_edge_list())
---
WeightedEdgeList[(0, 1, a), (0, 1, a)]
@georgios-ts georgios-ts added the bug Something isn't working label Aug 31, 2021
@georgios-ts
Copy link
Collaborator Author

Oh, actually i'm a bit confused now. I was ready to push a fix for this but then i realized i am not sure what's the expected output for this function. I was under the impression that merge_edges=True makes sense only if you set merge_nodes=True which in this case means "if two nodes of the second graph are connected by an edge and got merged into nodes of the first graph, do not add an extra edge if the last nodes have an edge with equal weight data". But we have a test case where this assumption does not hold: https://github.com/Qiskit/retworkx/blob/main/tests/digraph/test_union.py#L34

@mtreinish Any input on this?

@mtreinish
Copy link
Member

Yeah, I'm not sure exactly what the intent there is, based on that test it looks like it combines the edges if they're between nodes with the same weights and they have the same edge weight. Which seems a bit unexpected, like if the nodes aren't combined how do we know which edge to keep (just the first graph's)?

georgios-ts added a commit to georgios-ts/retworkx that referenced this issue Sep 7, 2021
Previously, `digraph_union` would falsely keep or delete
edges if `merge_edges` is set to true. This commit fixes
the logic of `digraph_union` to skip an edge from the second
graph if both its endpoints were merged to nodes from the
first graph and these nodes already share an edge with equal
weight data. At the same time, a new function `graph_union`
was added that returns the union of two `PyGraph`s.
Closes Qiskit#432.
mtreinish added a commit that referenced this issue Sep 8, 2021
* Fixes `digraph_union` if `merge_edges` is set to true.
Previously, `digraph_union` would falsely keep or delete
edges if `merge_edges` is set to true. This commit fixes
the logic of `digraph_union` to skip an edge from the second
graph if both its endpoints were merged to nodes from the
first graph and these nodes already share an edge with equal
weight data. At the same time, a new function `graph_union`
was added that returns the union of two `PyGraph`s.
Closes #432.

* increase test cov

* improve release note and message in panic exception

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* add dispatch function `union` and implement `find_node_by_weight` for `PyGraph`

* lint

* Release note fixes

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
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.

2 participants