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

Workaround isomorphism failure with node removals #115

Merged
merged 7 commits into from
Aug 25, 2020

Conversation

mtreinish
Copy link
Member

The dag_isomorphism module was forked from upstream petgraph to handle
the PyDiGraph type and also to enable handling exceptions in the python
check functions gracefully. However, when this was done it neglected
that here were limitations with that module which causes failures in
certain scenarios after node removals. This was because the upstream
petgraph implementation was built on the Graph type instead of the
StableGraph type. The only difference between these types is that
StableGraph does not reuse indexes on removals but Graph does. This
can cause there to be holes in the list of node ids. This breaks
assumptions in multiple places of the VF2 implementation causing a
panic if isomorphism checks are run on a PyDiGraph that has nodes
removed. This commit worksaround this limitation by checking if we've
removed nodes from the PyDiGraph object and if we have it iterates over
the graph and clones it into a copy with a condensed set of node ids.

This fix is less than ideal in that it results in a copy of the graph
which will potentially have performance implications, especially for
larger graphs. But after attempting to fix the VF2 implementation that
seems to be a more involved project than I originally hoped. This will
at least workaround the bug until a better more robust VF2
implementation can be written (and likely should be contributed back
upstream to petgraph).

Fixes #27

The dag_isomorphism module was forked from upstream petgraph to handle
the PyDiGraph type and also to enable handling exceptions in the python
check functions gracefully. However, when this was done it neglected
that here were limitations with that module which causes failures in
certain scenarios after node removals. This was because the upstream
petgraph implementation was built on the Graph type instead of the
StableGraph type. The only difference between these types is that
StableGraph does not reuse indexes on removals but Graph does. This
can cause there to be holes in the list of node ids. This breaks
assumptions in multiple places of the VF2 implementation causing a
panic if isomorphism checks are run on a PyDiGraph that has nodes
removed. This commit worksaround this limitation by checking if we've
removed nodes from the PyDiGraph object and if we have it iterates over
the graph and clones it into a copy with a condensed set of node ids.

This fix is less than ideal in that it results in a copy of the graph
which will potentially have performance implications, especially for
larger graphs. But after attempting to fix the VF2 implementation that
seems to be a more involved project than I originally hoped. This will
at least workaround the bug until a better more robust VF2
implementation can be written (and likely should be contributed back
upstream to petgraph).

Fixes Qiskit#27
@mtreinish
Copy link
Member Author

@georgesbarron this should fix the issue and remove the need to always deepcopy in QuantumCircuit.__eq__ If you could test this for the case where you were seeing performance impact I'm curious to see how much this mitigates any issues. This is slower than the ideal fix but since it's self contained in the rust code (except to increase the python ref counter as part of the copy) but should still be faster than unconditionally deepcopying. Also, if there are no removals on the dags being compared then this will be as fast as the ideal fix.

@coveralls
Copy link

coveralls commented Aug 18, 2020

Coverage Status

Coverage increased (+0.9%) to 91.447% when pulling 72856d5 on mtreinish:fix-isomorphism into d321588 on Qiskit:master.

@mtreinish
Copy link
Member Author

I did a quick synthetic test of the performance of this and removing the deepcopy from terra's dagcircuit by adding together to aqua operators:

import time
from qiskit.aqua.operators import X, Y, Z, I, CX, T, H

c_op = (((CX ^ 3) ^ X) @
        (H ^ 7) @
        (X ^ Y ^ Z ^ I ^ X ^ X ^ X) @
        (Y ^ (CX ^ 3)) @
        (X ^ Y ^ Z ^ I ^ X ^ X ^ X))
newop = (((CX ^ 3) ^ X) @
         (H ^ 7) @
         (X ^ Y ^ Z ^ I ^ X ^ X ^ X) @
         (Y ^ (CX ^ 3)) @
         (X ^ Y ^ Z ^ I ^ X ^ X ^ X))
start = time.time()
c_op + newop
stop = time.time()
print(stop - start)

With this PR (and the deepcopy removal from terra) the timed line took 0.005 seconds with qiskit 0.20.0 and retworkx 0.4.0 the timed line took 0.015 seconds. So it's definitely going to fix the issue reported in #27 (although it'd be good to confirm that with a more realistic example).

If you'd like to try and recreate this locally, the terra patch:

diff --git a/qiskit/dagcircuit/dagcircuit.py b/qiskit/dagcircuit/dagcircuit.py
index b98b4092..81c958dd 100644
--- a/qiskit/dagcircuit/dagcircuit.py
+++ b/qiskit/dagcircuit/dagcircuit.py
@@ -773,12 +773,8 @@ class DAGCircuit:
         return full_pred_map, full_succ_map
 
     def __eq__(self, other):
-        # TODO remove deepcopy calls after
-        # https://github.com/mtreinish/retworkx/issues/27 is fixed
-        slf = copy.deepcopy(self._multi_graph)
-        oth = copy.deepcopy(other._multi_graph)
-
-        return rx.is_isomorphic_node_match(slf, oth,
+        return rx.is_isomorphic_node_match(self._multi_graph,
+                                           other._multi_graph,
                                            DAGNode.semantic_eq)
 
     def topological_nodes(self):

then I ran:

virtualenv test-no-deepcopy
test-no-deepcopy/bin/pip install -e $PATH_TO_TERRA_WITH_DIFF
test-no-deepcopy/bin/pip install git+https://github.com/mtreinish/retworkx@fix-isomorphism
test-no-deepcopy/bin/pip install qiskit-aer qiskit-ignis qiskit-aqua
test-no-deepcopy/bin/python time_circuitop_add.py

(this does require rust be installed)

src/dag_isomorphism.rs Outdated Show resolved Hide resolved
@mtreinish mtreinish merged commit f4af5b0 into Qiskit:master Aug 25, 2020
@mtreinish mtreinish deleted the fix-isomorphism branch August 25, 2020 18:57
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

Successfully merging this pull request may close these issues.

dag_isomorphism::is_isomorphic_matching checks don't work on PyDAG with removed node
5 participants