Fix UnitaryGate.qasm() with unused qubits
#8234
Merged
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
The OpenQASM 2 exporter has a special case for providing `gate` definitions, which was implemented purely for `UnitaryGate`. This is stateful per instruction, and does not generally behave well with the rest of the exporter. This commit fixes one misbehaviour: the way the qubit operands for the definition was calculated was unreliable in both order and number of qubits, if any of the qubits were unaffected by the gate operation.
jakelishman
added
the
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:
|
mtreinish
added
the
stable backport potential
mtreinish
approved these changes
Jun 24, 2022
Pull Request Test Coverage Report for Build 2557223060
💛 - Coveralls |
mergify bot
pushed a commit
that referenced
this pull request
Jun 24, 2022
The OpenQASM 2 exporter has a special case for providing `gate` definitions, which was implemented purely for `UnitaryGate`. This is stateful per instruction, and does not generally behave well with the rest of the exporter. This commit fixes one misbehaviour: the way the qubit operands for the definition was calculated was unreliable in both order and number of qubits, if any of the qubits were unaffected by the gate operation. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 8d5e3ca)
mergify bot
added a commit
that referenced
this pull request
Jun 24, 2022
The OpenQASM 2 exporter has a special case for providing `gate` definitions, which was implemented purely for `UnitaryGate`. This is stateful per instruction, and does not generally behave well with the rest of the exporter. This commit fixes one misbehaviour: the way the qubit operands for the definition was calculated was unreliable in both order and number of qubits, if any of the qubits were unaffected by the gate operation. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 8d5e3ca) Co-authored-by: Jake Lishman <jake.lishman@ibm.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
| @@ -191,22 +191,13 @@ def qasm(self): | |||
| _qasm_escape_gate_name(self.label) if self.label else "unitary" + str(id(self)) | |||
| ) | |||
|
|
|||
| # map from gates in the definition to params in the method | |||
| bit_to_qasm = OrderedDict() | |||
There was a problem hiding this comment.
The OrderedDict() class is no longer being used, it should be better to remove that import to avoid any import issues.
There was a problem hiding this comment.
This is giving an error in further tests done using tox -elint due to it being an unused-import and can cause problems for future developers when running tests.
There was a problem hiding this comment.
Thanks, this was a good catch. I'm not sure how this slipped through CI here (I relied a bit too heavily on it in my code review earlier) but this also just started failing on CI today, the fix for this is here: #8366
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Jul 18, 2022
Recently the pylint CI runs have all started failing because of an unused OrderedDict import failure in qiskit/extensions/unitary.py. The OrderedDict usage was removed in Qiskit#8234 but the import was left in accidently. This should have been caught by the lint job on that PR but for some reason it was not caught and has sat there until relatively recently when pylint started erroring because of the error. Normally for failures like this they can be attributed to environment differences, typically a release of pylint or astroid. However, we pin those package versions because of their tendancy to change behavior, and also a diff between the installed python packages in CI doesn't show any differences. Regardless of the underlying cause to unblock CI this commit is necessary to fix the unused import error.
Cryoris
pushed a commit
that referenced
this pull request
Jul 19, 2022
Recently the pylint CI runs have all started failing because of an unused OrderedDict import failure in qiskit/extensions/unitary.py. The OrderedDict usage was removed in #8234 but the import was left in accidently. This should have been caught by the lint job on that PR but for some reason it was not caught and has sat there until relatively recently when pylint started erroring because of the error. Normally for failures like this they can be attributed to environment differences, typically a release of pylint or astroid. However, we pin those package versions because of their tendancy to change behavior, and also a diff between the installed python packages in CI doesn't show any differences. Regardless of the underlying cause to unblock CI this commit is necessary to fix the unused import error.
mergify bot
pushed a commit
that referenced
this pull request
Jul 19, 2022
Recently the pylint CI runs have all started failing because of an unused OrderedDict import failure in qiskit/extensions/unitary.py. The OrderedDict usage was removed in #8234 but the import was left in accidently. This should have been caught by the lint job on that PR but for some reason it was not caught and has sat there until relatively recently when pylint started erroring because of the error. Normally for failures like this they can be attributed to environment differences, typically a release of pylint or astroid. However, we pin those package versions because of their tendancy to change behavior, and also a diff between the installed python packages in CI doesn't show any differences. Regardless of the underlying cause to unblock CI this commit is necessary to fix the unused import error. (cherry picked from commit 007a746)
mergify bot
added a commit
that referenced
this pull request
Jul 19, 2022
Recently the pylint CI runs have all started failing because of an unused OrderedDict import failure in qiskit/extensions/unitary.py. The OrderedDict usage was removed in #8234 but the import was left in accidently. This should have been caught by the lint job on that PR but for some reason it was not caught and has sat there until relatively recently when pylint started erroring because of the error. Normally for failures like this they can be attributed to environment differences, typically a release of pylint or astroid. However, we pin those package versions because of their tendancy to change behavior, and also a diff between the installed python packages in CI doesn't show any differences. Regardless of the underlying cause to unblock CI this commit is necessary to fix the unused import error. (cherry picked from commit 007a746) Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
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.
The OpenQASM 2 exporter has a special case for providing
gatedefinitions, which was implemented purely for
UnitaryGate. This isstateful per instruction, and does not generally behave well with the
rest of the exporter. This commit fixes one misbehaviour: the way the
qubit operands for the definition was calculated was unreliable in both
order and number of qubits, if any of the qubits were unaffected by
the gate operation.
Details and comments
There are general problems with the special-case approach to defining the definition like this (see also #8222), which this commit does not fix, and the whole OpenQASM 2 exporter is known to be pretty questionable with custom and parameterised gates still.
Fix #8224.