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.

Sign up for GitHub

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

Jump to bottom

Fix deepcopy for new-style Bit #10411

Merged
merged 4 commits into from
Jul 11, 2023
Merged

Fix deepcopy for new-style Bit #10411

merged 4 commits into from
Jul 11, 2023

Conversation

kevinhartman
Copy link
Contributor

@kevinhartman kevinhartman commented Jul 10, 2023 •
edited
Loading

Summary

Overrides the default behavior of copy and deepcopy for Bit to return self for new-style bits.

Details and comments

We choose this behavior because Bits are immutable, and it allows us to deep-copy new-style bits while maintaining equality between the original and the copy. For old-style bits, we leave the behavior unchanged, since some existing code appears to depend on _register getting deep-copied.

Note that we now return self when shallow-copying old-style bits, which we can do safely since Bits themselves are immutable.

Resolves #10409.

@kevinhartman kevinhartman 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 labels Jul 10, 2023
@kevinhartman kevinhartman added this to the 0.24.2 milestone Jul 10, 2023
@kevinhartman kevinhartman requested a review from a team as a code owner July 10, 2023 15:42
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

jakelishman
jakelishman reviewed Jul 10, 2023
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

This looks sensible to me, thanks. I think it would be good to insert a test, if there's a sensible one we can do.

releasenotes/notes/fix-bit-copy-4b2f7349683f616a.yaml Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 10, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5513114945

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • 159 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.008%) to 85.994%
Files with Coverage Reduction New Missed Lines %
qiskit/qasm/init.py 1 80.0%
crates/qasm2/src/lex.rs 2 91.39%
qiskit/extensions/quantum_initializer/squ.py 2 80.0%
qiskit/qasm/pygments/init.py 3 0%
qiskit/qpy/binary_io/circuits.py 11 92.95%
qiskit/qasm/pygments/lexer.py 14 0%
qiskit/tools/jupyter/library.py 43 0%
qiskit/circuit/quantumcircuit.py 83 94.05%
Totals Coverage Status
Change from base Build 5490783039: 0.008%
Covered Lines: 71629
Relevant Lines: 83295
💛 - Coveralls

@kevinhartman kevinhartman mentioned this pull request Jul 10, 2023
Closed
@kevinhartman
Copy link
Contributor Author

kevinhartman commented Jul 10, 2023 •
edited
Loading

I've updated the description since the PR now only changes the behavior of deepcopy for new-style bits.

@kevinhartman kevinhartman changed the title Make Bit copy return self. Fix deepcopy for new-style Bit Jul 10, 2023
jakelishman
jakelishman approved these changes Jul 11, 2023
View reviewed changes
Copy link
Member

@jakelishman jakelishman 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, thanks for the spotting the problem and solving it!

qiskit/circuit/bit.py
Comment on lines +133 to +140
# Old-style bits need special handling for now, since some code seems
# to rely on their registers getting deep-copied.
bit = type(self).__new__(type(self))
bit._register = copy.deepcopy(self._register, memo)
bit._index = self._index
bit._hash = self._hash
bit._repr = self._repr
return bit
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, in fairness, we were wrong before when we didn't recurse the deepcopy onto the register. It likely would have caused a weird inconsistency where the reg[0]._register is not reg, which is an invariant of old-style registers/bits at the moment. I can't immediately see why that has knock-on effects, but I guess it shouldn't be surprising.

@jakelishman jakelishman added this pull request to the merge queue Jul 11, 2023
Merged via the queue into Qiskit:main with commit e9c86a5 Jul 11, 2023
13 checks passed
mergify bot pushed a commit that referenced this pull request Jul 11, 2023
@kevinhartman @mergify
Fix deepcopy for new-style Bit (#10411)
66f0fcc 
* Make bit copy return self.

* Add release note.

* Preserve existing deepcopy behavior for old bits.

* Add tests to validate copy behaviors.

(cherry picked from commit e9c86a5)
@mergify mergify bot mentioned this pull request Jul 11, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jul 11, 2023
@mergify @kevinhartman
Fix deepcopy for new-style Bit (#10411) (#10413)
76d52cb 
* Make bit copy return self.

* Add release note.

* Preserve existing deepcopy behavior for old bits.

* Add tests to validate copy behaviors.

(cherry picked from commit e9c86a5)

Co-authored-by: Kevin Hartman <kevin@hart.mn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakelishman jakelishman jakelishman approved these changes

Assignees
No one assigned
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Milestone
0.24.2
Development

Successfully merging this pull request may close these issues.

Deepcopy of control flow with standalone clbit results in invalid circuit
4 participants
@kevinhartman @qiskit-bot @coveralls @jakelishman