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

Fix copying control-flow bodies in builder interface #7368

Merged

Conversation

jakelishman
Copy link
Member

Summary

There are a few places where the structure of an element of
QuantumCircuit.data is specifically assumed to be

Tuple[Instruction, List[Qubit], List[Clbit]]

rather than the more abstract

Tuple[Instruction, Sequence[Qubit], Sequence[Clbit]]

For performance reasons, the control-flow builders used tuples to store
their arguments internally, because this prevents any copying being
necessary, and allows the arguments to be put in sets. Now, in lieu of
changing the QuantumCircuit data structure to be more abstract, which
would have performance implications, or to require the tuples, which may
create more bugs, we just make the builder interface convert its tuples
to lists on output.

Details and comments

A relatively hidden bug, but a bug nonetheless, and fixing this will make it much easier for Aer to write its test cases for 0.10. @hhorii.

Fix #7367.

There are a few places where the structure of an element of
`QuantumCircuit.data` is specifically assumed to be

    Tuple[Instruction, List[Qubit], List[Clbit]]

rather than the more abstract

    Tuple[Instruction, Sequence[Qubit], Sequence[Clbit]]

For performance reasons, the control-flow builders used tuples to store
their arguments internally, because this prevents any copying being
necessary, and allows the arguments to be put in sets.  Now, in lieu of
changing the `QuantumCircuit` data structure to be more abstract, which
would have performance implications, or to require the tuples, which may
create more bugs, we just make the builder interface convert its tuples
to lists on output.
@jakelishman jakelishman added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog circuit-builder labels Dec 7, 2021
@jakelishman jakelishman requested a review from a team as a code owner December 7, 2021 16:55
@coveralls
Copy link

coveralls commented Dec 7, 2021

Pull Request Test Coverage Report for Build 1551115436

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.008%) to 83.094%

Files with Coverage Reduction New Missed Lines %
qiskit/extensions/quantum_initializer/uc.py 2 88.65%
qiskit/pulse/library/waveform.py 3 89.36%
Totals Coverage Status
Change from base Build 1550571358: -0.008%
Covered Lines: 51667
Relevant Lines: 62179

💛 - Coveralls

@jakelishman jakelishman added this to the 0.19.1 milestone Dec 7, 2021
Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

LGTM!

@mergify mergify bot merged commit d38620a into Qiskit:main Dec 7, 2021
mergify bot pushed a commit that referenced this pull request Dec 7, 2021
There are a few places where the structure of an element of
`QuantumCircuit.data` is specifically assumed to be

    Tuple[Instruction, List[Qubit], List[Clbit]]

rather than the more abstract

    Tuple[Instruction, Sequence[Qubit], Sequence[Clbit]]

For performance reasons, the control-flow builders used tuples to store
their arguments internally, because this prevents any copying being
necessary, and allows the arguments to be put in sets.  Now, in lieu of
changing the `QuantumCircuit` data structure to be more abstract, which
would have performance implications, or to require the tuples, which may
create more bugs, we just make the builder interface convert its tuples
to lists on output.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit d38620a)
mergify bot added a commit that referenced this pull request Dec 8, 2021
There are a few places where the structure of an element of
`QuantumCircuit.data` is specifically assumed to be

    Tuple[Instruction, List[Qubit], List[Clbit]]

rather than the more abstract

    Tuple[Instruction, Sequence[Qubit], Sequence[Clbit]]

For performance reasons, the control-flow builders used tuples to store
their arguments internally, because this prevents any copying being
necessary, and allows the arguments to be put in sets.  Now, in lieu of
changing the `QuantumCircuit` data structure to be more abstract, which
would have performance implications, or to require the tuples, which may
create more bugs, we just make the builder interface convert its tuples
to lists on output.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit d38620a)

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@jakelishman jakelishman deleted the fix/control-flow-builder-parameter-copy branch December 10, 2021 09:21
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 circuit-builder stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Control-flow "parameter" circuits cannot be copied
3 participants