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

[Stable] Improve 1q decomposition pass with multiple matching basis (#5431) #5655

Merged
merged 8 commits into from
Jan 21, 2021

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Jan 19, 2021

Summary

  • Improve 1q decomposition pass with multiple matching basis

This commit makes an improvement to the Optimize1qGatesDecomposition
pass to improve the output from it in two scenarios. The first scenario
is if there is an over complete basis where there are multiple choices
for decomposition basis. Previously in such scenarios only the first
matching subset would be used. This improves that by finding runs in all
matching decomposition basis to simplify any runs in those gate sets.
The second improvement made here is that the decomposition is only used
if it results in a decrease in depth. There were scenarios where the
general decomposition of a run would result in more gates than the
input to the OneQubitEulerDecomposer (typically 3 gates vs 2 input). In
those cases we shouldn't use the decomposition and instead rely on the
original run because we're adding gates which is the opposit of the
expected behavior.

  • Add DAGCircuit method to get 1q op node runs

This commit adds a new DAGCircuit method, collect_1q_runs, which behaves
like collect_runs(), but instead of collecting runs with gates on a name
filter it looks at the number of qubits and returns runs of any op node
that operate on a single qubit. This will be used by the optimize 1q
decomposition pass to collect arbitrary single qubit gate runs and
decompose them over the basis sets.

  • Switch to use all 1q runs instead of those just matching basis

  • Move parameterized gate split to collect_1q_runs

  • Reverse loops so basis is the inner loop and runs the outer

  • Ensure collect_1q_runs doesn't collect measurements

  • Only create temp circuit once per run

  • Move lone identity optimization into outer loop

  • Update qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py

  • Fix lint

  • Use qc._append instead of qc.append

Details and comments

Backported from #5431 and #5570

Conflicts/Changes:

For backport this makes 1 key change, the collect_1q_runs method is
rewritten to be pure python instead of leveraging the retworkx
collect_runs() method which was added in a newer version of retworkx
than the minimum allowed version. This also partially pulls in the
changes made in #5570, but only some were backportable since the others
relied on attributes that don't exist in gate objects in 0.16.x.

(cherry picked from commit 5a1768f)

* Improve 1q decomposition pass with multiple matching basis

This commit makes an improvement to the Optimize1qGatesDecomposition
pass to improve the output from it in two scenarios. The first scenario
is if there is an over complete basis where there are multiple choices
for decomposition basis. Previously in such scenarios only the first
matching subset would be used. This improves that by finding runs in all
matching decomposition basis to simplify any runs in those gate sets.
The second improvement made here is that the decomposition is only used
if it results in a decrease in depth. There were scenarios where the
general decomposition of a run would result in more gates than the
input to the OneQubitEulerDecomposer (typically 3 gates vs 2 input). In
those cases we shouldn't use the decomposition and instead rely on the
original run because we're adding gates which is the opposit of the
expected behavior.

* Add DAGCircuit method to get 1q op node runs

This commit adds a new DAGCircuit method, collect_1q_runs, which behaves
like collect_runs(), but instead of collecting runs with gates on a name
filter it looks at the number of qubits and returns runs of any op node
that operate on a single qubit. This will be used by the optimize 1q
decomposition pass to collect arbitrary single qubit gate runs and
decompose them over the basis sets.

* Switch to use all 1q runs instead of those just matching basis

* Use retworkx.collect_runs for collect_1q_runs

* Move parameterized gate split to collect_1q_runs

* Reverse loops so basis is the inner loop and runs the outer

* Ensure collect_1q_runs doesn't collect measurements

* Only create temp circuit once per run

* Move lone identity optimization into outer loop

* Update qiskit/transpiler/passes/optimization/optimize_1q_decomposition.py

* Fix lint

* Use qc._append instead of qc.append

Conflicts/Changes:

For backport this makes 1 key change, the collect_1q_runs method is
rewritten to be pure python instead of leveraging the retworkx
collect_runs() method which was added in a newer version of retworkx
than the minimum allowed version. This also partially pulls in the
changes made in Qiskit#5570, but only some were backportable since the others
relied on attributes that don't exist in gate objects in 0.16.x.

(cherry picked from commit 5a1768f)
@mtreinish mtreinish requested a review from a team as a code owner January 19, 2021 17:46
@mtreinish mtreinish changed the title Improve 1q decomposition pass with multiple matching basis (#5431) [Stable] Improve 1q decomposition pass with multiple matching basis (#5431) Jan 19, 2021
@mtreinish mtreinish added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Jan 19, 2021
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Does this need its own release note?

qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
qiskit/dagcircuit/dagcircuit.py Outdated Show resolved Hide resolved
@mtreinish mtreinish requested a review from kdk January 19, 2021 22:22
@mtreinish
Copy link
Member Author

mtreinish commented Jan 19, 2021

@kdk Ok, I've updated this per your comments. As for the release notes I'll defer to you on it. I didn't write one for the master commit because I felt it was an internal implementation detail and kind of hard to describe the difference in a user facing perspective besides something like "update the 1q optimization decompose pass so it does a better job for an overcomplete basis" which didn't seem necessarily helpful as a fix note.

@mtreinish mtreinish requested a review from kdk January 20, 2021 22:42
@kdk kdk added the automerge label Jan 20, 2021
@mergify mergify bot merged commit dc7ac53 into Qiskit:stable/0.16 Jan 21, 2021
@mtreinish mtreinish deleted the backport-improve-1q-decomp branch January 21, 2021 15:18
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Jan 22, 2021
The collect_1q_runs method was rewritten in python as part of the
backport of a larger bugfix in the Optimize1qGatesDecomposition
transpiler pass in Qiskit#5655. However, it is not part of the public api in
0.16.x and was only backported to fix the bug in
Optimize1qGatesDecomposition. Since the version included on the master
branch will be subtlely different in return type (after Qiskit#5685) Since we
don't want to advertise the method until it's included in the public api
this commit prepends '_' making the method name _collect_1q_runs to
indicate it is internal only as part of the 0.16.2 release.
mtreinish added a commit that referenced this pull request Jan 26, 2021
* Prepare 0.16.2 release

This commit prepares for the 0.16.2 release by bumping the version
strings to reflect the new version. This is just a bugfix release
that several fixes for bugs introduced in 0.16.1.

* Mark DAGCircuit collect_1q_runs private

The collect_1q_runs method was rewritten in python as part of the
backport of a larger bugfix in the Optimize1qGatesDecomposition
transpiler pass in #5655. However, it is not part of the public api in
0.16.x and was only backported to fix the bug in
Optimize1qGatesDecomposition. Since the version included on the master
branch will be subtlely different in return type (after #5685) Since we
don't want to advertise the method until it's included in the public api
this commit prepends '_' making the method name _collect_1q_runs to
indicate it is internal only as part of the 0.16.2 release.

* Cleanup release notes
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Jan 28, 2021
This commit fixes a bug introduced in the backport PR Qiskit#5655. On the
current master branch the bugfix that was being backported relies on a
feature in newer versions of retworkx. Since we can't bump the minimum
retworkx version required on a stable branch that functionality was
rewritten in the dagcircuit class directly. However, that implementation
included a typo which is causing non-gates and atomic instructions to be
included in collected runs which is not correct behavior and causes
errors to be raised. This commit corrects the bug in the backport and
adds a test to ensure the regression is not present.

This is a direct commit to the stable branch and not a backport since
this bug is not present on master and was only introduced through a bad
backport.

Fixes Qiskit#5736
mergify bot pushed a commit that referenced this pull request Jan 28, 2021
This commit fixes a bug introduced in the backport PR #5655. On the
current master branch the bugfix that was being backported relies on a
feature in newer versions of retworkx. Since we can't bump the minimum
retworkx version required on a stable branch that functionality was
rewritten in the dagcircuit class directly. However, that implementation
included a typo which is causing non-gates and atomic instructions to be
included in collected runs which is not correct behavior and causes
errors to be raised. This commit corrects the bug in the backport and
adds a test to ensure the regression is not present.

This is a direct commit to the stable branch and not a backport since
this bug is not present on master and was only introduced through a bad
backport.

Fixes #5736
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants