Fix QuantumCircuit.compose within control-flow scopes #8570
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
Previously, `QuantumCircuit.compose` with `inplace=True` would insert the appended instructions outside the scope due to unchecked use of `QuantumCircuit._append` (and direct modification of the `data` attribute). This now respects any active control-flow scopes. Using `front=True` in conjunction with `inplace=True` within a control-flow scope has no clear meaning, and so the behaviour is forbidden. Emitting a new circuit (`inplace=False`) while in an active scope would produce a broken circuit - the active scope would need to be cloned to ensure equivalence, but it wouldn't be part of any Python-managed context, so the scope could never be closed.
jakelishman
added
the
Changelog: Bugfix
This comment was marked as duplicate.
This comment was marked as duplicate.
mtreinish
reviewed
Aug 17, 2022
Pull Request Test Coverage Report for Build 2882454557
💛 - Coveralls |
mtreinish
reviewed
Aug 17, 2022
mtreinish
approved these changes
Aug 18, 2022
mtreinish
added
stable backport potential
mergify bot
pushed a commit
that referenced
this pull request
Aug 18, 2022
* Fix QuantumCircuit.compose within control-flow scopes Previously, `QuantumCircuit.compose` with `inplace=True` would insert the appended instructions outside the scope due to unchecked use of `QuantumCircuit._append` (and direct modification of the `data` attribute). This now respects any active control-flow scopes. Using `front=True` in conjunction with `inplace=True` within a control-flow scope has no clear meaning, and so the behaviour is forbidden. Emitting a new circuit (`inplace=False`) while in an active scope would produce a broken circuit - the active scope would need to be cloned to ensure equivalence, but it wouldn't be part of any Python-managed context, so the scope could never be closed. * Finish writing documentation sentence * Add missing docstring in test * Add missing Sphinx directive Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 26ba58a)
jlapeyre
reviewed
Aug 18, 2022
There was a problem hiding this comment.
I like putting the description and constraints in one place. And making more explicit what is valid input. Eg, "must not" in place of "not possible"
The google style guide and examples (which we claim to follow) support this preference. The rule is stated here.
Here is an example in the google style guide. In this case the docstring says minimum must be a "port value greater than 1024". An exception is raised if this is violated, but there is no entry for this in the Raises section. There is an entry in Raises for an exception raised if a port is not available.
EDIT: I see this was merged while I was writing this review !
| @@ -835,7 +835,8 @@ def compose( | |||
| this can be anything that :obj:`.append` will accept. | |||
| qubits (list[Qubit|int]): qubits of self to compose onto. | |||
| clbits (list[Clbit|int]): clbits of self to compose onto. | |||
| front (bool): If True, front composition will be performed (not implemented yet). | |||
| front (bool): If True, front composition will be performed. This is not possible within | |||
There was a problem hiding this comment.
This might be better:
... This must not be
Trueif a control-flow builder block is active as there is no clear meaning to this action.
Then remove the corresponding entry in the Raises section below.
mergify bot
added a commit
that referenced
this pull request
Aug 18, 2022
* Fix QuantumCircuit.compose within control-flow scopes Previously, `QuantumCircuit.compose` with `inplace=True` would insert the appended instructions outside the scope due to unchecked use of `QuantumCircuit._append` (and direct modification of the `data` attribute). This now respects any active control-flow scopes. Using `front=True` in conjunction with `inplace=True` within a control-flow scope has no clear meaning, and so the behaviour is forbidden. Emitting a new circuit (`inplace=False`) while in an active scope would produce a broken circuit - the active scope would need to be cloned to ensure equivalence, but it wouldn't be part of any Python-managed context, so the scope could never be closed. * Finish writing documentation sentence * Add missing docstring in test * Add missing Sphinx directive Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 26ba58a) Co-authored-by: Jake Lishman <jake.lishman@ibm.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
Previously,
QuantumCircuit.composewithinplace=Truewould insertthe appended instructions outside the scope due to unchecked use of
QuantumCircuit._append(and direct modification of thedataattribute). This now respects any active control-flow scopes.
Using
front=Truein conjunction withinplace=Truewithin acontrol-flow scope has no clear meaning, and so the behaviour is
forbidden. Emitting a new circuit (
inplace=False) while in an activescope would produce a broken circuit - the active scope would need to be
cloned to ensure equivalence, but it wouldn't be part of any
Python-managed context, so the scope could never be closed.
Details and comments
Fix #8433.