-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 CollectCliffords
to index nodes using their ids
#13484
base: main
Are you sure you want to change the base?
Conversation
Needed because this field is named differently in DAGOpNode and DAGDepNode.
One or more of the following people are relevant to this code:
|
CollectCliffords
to index nodes using their ids
Pull Request Test Coverage Report for Build 11994394990Details
💛 - Coveralls |
@@ -54,7 +54,7 @@ def __init__(self, dag: DAGCircuit | DAGDependency): | |||
|
|||
self.dag = dag | |||
self._pending_nodes: list[DAGOpNode | DAGDepNode] | None = None | |||
self._in_degree: dict[DAGOpNode | DAGDepNode, int] | None = None | |||
self._in_degree: dict[int, int] | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is problematic, using the node ids is only safe for a single dag. If we want to switch this to be keyed on node ids we can not cache it. We could get away with the cache with the DAGNode
storage because the hash and eq implementations are unique; either pre-rust dagcircuit where it was object id based or post-rust dag where this is an actual implementation. But node ids are not unique between dags and the indices are reused between dags and will refer to different nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the point, but a single BlockCollector
class only makes sense for a single DAG, in which case we should be fine using node ids, correct?
Summary
Fixes #13457.
This PR changes the internal field
in_degree
inBlockCollector
to be indexed by the ids of the nodes rather than by nodes themselves. See also #13424 for a related discussion.In particular, this avoids hashing the nodes as python objects, both making the code faster and avoiding problems for things like
PauliLindbladError
coming fromqiskit_aer
.Details and comments
I am not sure if this change really deserves a release note, but I have decided to add one, given that it does address a reported bug.