-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Do not deepcopy operations in Unroll3qOrMore #10702
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the Unroll3qOrMore pass internally is quite simple it iterates over every operation in the circuit and for any operation that uses >= 3 qubits and recursively decompsing it into all 1 and 2 qubit operations, converting it to a DAGCircuit and substituting the >=3 qubit gates with the equivalent circuit. However, during this DAGCircuit conversion step we're spending a large amount deep copying the operation objects. However, we don't need to do this because nothing in the circuit will be reused with shared references so we can skip the copying and just pass the operation objects by reference onto the DAG (as that's all we need). This commit makes that change by using the `copy_operations` flag we introduced in Qiskit#9848 on circuit_to_dag() to disable the internal copying.
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 5955802704
💛 - Coveralls |
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Aug 24, 2023
This commit adds a cache for parameter free gate decompositions in the Unroll3qOrMore transpiler pass. If there are repeated gates in a circuit that don't have any parameters, then when we decompose the circuit a single time that decomposition will be valid for all instances of that gate. This doesn't hold true for parameterized gates because the parameter value will determine the decomposition used. This greatly improves the runtime performance of the pass when there are repeated >=3 qubit gates in the circuit. In a future revision we can investigate adding the parameter values to the cache lookup. But the hit rate would likely be fairly low in such cases because it would only provide runtime improvements if there are repeated >3 qubit gates with the same exact parameter values, which seems fairly rare in practice. This is based on top of Qiskit#10702 and will need to be rebased after Qiskit#10702 merges.
jlapeyre
approved these changes
Aug 25, 2023
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.
Looks fine.
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Aug 25, 2023
This commit adds a cache for parameter free gate decompositions in the Unroll3qOrMore transpiler pass. If there are repeated gates in a circuit that don't have any parameters, then when we decompose the circuit a single time that decomposition will be valid for all instances of that gate. This doesn't hold true for parameterized gates because the parameter value will determine the decomposition used. This greatly improves the runtime performance of the pass when there are repeated >=3 qubit gates in the circuit. In a future revision we can investigate adding the parameter values to the cache lookup. But the hit rate would likely be fairly low in such cases because it would only provide runtime improvements if there are repeated >3 qubit gates with the same exact parameter values, which seems fairly rare in practice. This is based on top of Qiskit#10702 and will need to be rebased after Qiskit#10702 merges.
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Aug 25, 2023
This commit adds a cache for parameter free gate decompositions in the Unroll3qOrMore transpiler pass. If there are repeated gates in a circuit that don't have any parameters, then when we decompose the circuit a single time that decomposition will be valid for all instances of that gate. This doesn't hold true for parameterized gates because the parameter value will determine the decomposition used. This greatly improves the runtime performance of the pass when there are repeated >=3 qubit gates in the circuit. In a future revision we can investigate adding the parameter values to the cache lookup. But the hit rate would likely be fairly low in such cases because it would only provide runtime improvements if there are repeated >3 qubit gates with the same exact parameter values, which seems fairly rare in practice. This is based on top of Qiskit#10702 and will need to be rebased after Qiskit#10702 merges.
SameerD-phys
pushed a commit
to SameerD-phys/qiskit-terra
that referenced
this pull request
Sep 7, 2023
In the Unroll3qOrMore pass internally is quite simple it iterates over every operation in the circuit and for any operation that uses >= 3 qubits and recursively decompsing it into all 1 and 2 qubit operations, converting it to a DAGCircuit and substituting the >=3 qubit gates with the equivalent circuit. However, during this DAGCircuit conversion step we're spending a large amount deep copying the operation objects. However, we don't need to do this because nothing in the circuit will be reused with shared references so we can skip the copying and just pass the operation objects by reference onto the DAG (as that's all we need). This commit makes that change by using the `copy_operations` flag we introduced in Qiskit#9848 on circuit_to_dag() to disable the internal copying.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
In the Unroll3qOrMore pass internally is quite simple it iterates over every operation in the circuit and for any operation that uses >= 3 qubits and recursively decompsing it into all 1 and 2 qubit operations, converting it to a DAGCircuit and substituting the >=3 qubit gates with the equivalent circuit. However, during this DAGCircuit conversion step we're spending a large amount deep copying the operation objects. However, we don't need to do this because nothing in the circuit will be reused with shared references so we can skip the copying and just pass the operation objects by reference onto the DAG (as that's all we need). This commit makes that change by using the
copy_operations
flag we introduced in #9848 on circuit_to_dag() to disable the internal copying.Details and comments