Enable simplifying 1q runs and 2q blocks in transpile() without target #9222
Conversation
This file contains 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
This commit fixes an issue when transpile() was called with optimization enabled (optimization levels 1, 2, and 3) and no target (or basis gates) specified then 1q run or 2q block optimization was run. This would result in long series of gates that could be simplified being left in the output circuit. To address this, for 1q optimization the Optimize1qGatesDecomposition pass (which is run at all 3 optimization levels) is updated so that if no target is specified we just try all decomposers as the default heuristic for the best output is the shortest sequence length (in the absense of error rates from the target) and if any output gate is valid that will either remove the 1q sequence if it's an identity sequence, or likely be a single gate. As no basis is specified this behavior is fine since the calculations are quick and any output basis will match the constraints the user provided the transpiler. For optimization level 3 with it's 2q blcok optimization with the UnitarySynthesis pass it is a bit more involved though. The cost of doing the unitary synthesis is higher, the number of possible decompositions is larger, and we don't have a good heuristic measure of which would perform best without a target specified and it's not feasible to just try all supported basis by the synthesis module. This means for a non-identity 2 qubit block the output will be a UnitaryGate (which without a target specified is a valid output). However, to address the case when an identity block is present in the circuit this can be removed with very little overhead. To accomplish this the ConsolidateBlocks pass is updated to check if an identified 2 qubit block is equal the identity matrix and if so will remove that block from the circuit. Fixes Qiskit#9217
mtreinish
added
Changelog: Bugfix
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 3595486434
💛 - Coveralls |
mtreinish
force-pushed
the
fix-identity-simplification-no-target
branch
from
3b078b8 to
7f687ae
Compare
jakelishman
reviewed
Dec 1, 2022
Comment on lines
+118
to
+119
| identity = np.eye(2**unitary.num_qubits) | ||
| if np.allclose(identity, unitary.to_matrix()): |
There was a problem hiding this comment.
allclose has an implicit tolerance built-in, so we're potentially removing things that aren't precisely the identity here. That's sensible, of course, but maybe it's worth making sure the way we handle the tolerance here is consistent with how we treat approximation_degree (especially since that's caused issues in the past) and anywhere else that might skip small values (CommutationAnalysis, maybe?).
There was a problem hiding this comment.
Well I figured it was ok like this because there is a similar tolerance built-in to the synthesis routines that normally do this optimization that isn't really adjustable either. The approximation_degree usage for synthesis is used for the fidelity of the approximation of the synthesized circuit. I can try to wire the tolerance here in with the value for approximation_degree. I guess I'll just have to figure out what values make sense for a mapping of approximation degree which is from 0 to 1 to the tolerance parameters on allclose.
There was a problem hiding this comment.
If an implicit tolerance is something we do elsewhere, I'm happy enough to leave it in place here - I was just concerned with making sure the type of behaviour we have is consistent with itself, whatever it is.
There was a problem hiding this comment.
That makes sense, just for some references what I was talking about was:
- https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py#L114
- https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/quantum_info/synthesis/two_qubit_decompose.py has a bunch of
isclose()andallclose()calls. The tolerance is adjustable for some (but not all) and where it is adjustable that tolerance value is decoupled from theapproximation_degreevalue, which is use to set thebasis_fidelityargument when the actual decomposer is called.
qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py
Outdated
Show resolved
Hide resolved
releasenotes/notes/fix-identity-simplification-no-target-62cd8614044a0fe9.yaml
Outdated
Show resolved
Hide resolved
This commit fixes an oversight in the previous commit for the 1q decomposer pass when no basis gates are specified. Previously we were setting the basis list to be the set of all the gates in the supported euler basis for the decomposer. This had the unintended side effect of breaking the heuristic for the decomposer as it would treat any 1q gate outside of the supported euler basis as needing to be translated. This was not the desired behavior as any gate is valid. This fixes the logic to just treat no basis explicitly as everything being valid output and weight the heuristic error only.
jakelishman
approved these changes
Dec 1, 2022
Cryoris
pushed a commit
to Cryoris/qiskit-terra
that referenced
this pull request
Jan 12, 2023
Qiskit#9222) * Enable simplifying 1q runs and 2q blocks in transpile() without target This commit fixes an issue when transpile() was called with optimization enabled (optimization levels 1, 2, and 3) and no target (or basis gates) specified then 1q run or 2q block optimization was run. This would result in long series of gates that could be simplified being left in the output circuit. To address this, for 1q optimization the Optimize1qGatesDecomposition pass (which is run at all 3 optimization levels) is updated so that if no target is specified we just try all decomposers as the default heuristic for the best output is the shortest sequence length (in the absense of error rates from the target) and if any output gate is valid that will either remove the 1q sequence if it's an identity sequence, or likely be a single gate. As no basis is specified this behavior is fine since the calculations are quick and any output basis will match the constraints the user provided the transpiler. For optimization level 3 with it's 2q blcok optimization with the UnitarySynthesis pass it is a bit more involved though. The cost of doing the unitary synthesis is higher, the number of possible decompositions is larger, and we don't have a good heuristic measure of which would perform best without a target specified and it's not feasible to just try all supported basis by the synthesis module. This means for a non-identity 2 qubit block the output will be a UnitaryGate (which without a target specified is a valid output). However, to address the case when an identity block is present in the circuit this can be removed with very little overhead. To accomplish this the ConsolidateBlocks pass is updated to check if an identified 2 qubit block is equal the identity matrix and if so will remove that block from the circuit. Fixes Qiskit#9217 * Fix docs build * Fix handling of 1q decomposer logic with no basis gates This commit fixes an oversight in the previous commit for the 1q decomposer pass when no basis gates are specified. Previously we were setting the basis list to be the set of all the gates in the supported euler basis for the decomposer. This had the unintended side effect of breaking the heuristic for the decomposer as it would treat any 1q gate outside of the supported euler basis as needing to be translated. This was not the desired behavior as any gate is valid. This fixes the logic to just treat no basis explicitly as everything being valid output and weight the heuristic error only. * Remove debug prints * Remove unnecessary conditional 1q identity matrix creation * Split out release notes for 1q and 2q cases Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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
This commit fixes an issue when transpile() was called with optimization enabled (optimization levels 1, 2, and 3) and no target (or basis gates) specified then 1q run or 2q block optimization was run. This would result in long series of gates that could be simplified being left in the output circuit. To address this, for 1q optimization the
Optimize1qGatesDecomposition pass (which is run at all 3 optimization levels) is updated so that if no target is specified we just try all decomposers as the default heuristic for the best output is the shortest sequence length (in the absense of error rates from the target) and if any output gate is valid that will either remove the 1q sequence if it's an identity sequence, or likely be a single gate. As no basis is specified this behavior is fine since the calculations are quick and any output basis will match the constraints the user provided the transpiler.
For optimization level 3 with it's 2q blcok optimization with the UnitarySynthesis pass it is a bit more involved though. The cost of doing the unitary synthesis is higher, the number of possible decompositions is larger, and we don't have a good heuristic measure of which would perform best without a target specified and it's not feasible to just try all supported basis by the synthesis module. This means for a non-identity 2 qubit block the output will be a UnitaryGate (which without a target specified is a valid output). However, to address the case when an identity block is present in the circuit this can be removed with very little overhead. To accomplish this the ConsolidateBlocks pass is updated to check if an identified 2 qubit block is equal the identity matrix and if so will remove that block from the circuit.
Details and comments
Fixes #9217