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

Removal of _index and _registers from Bit #11596

Closed

Conversation

sbrandhsn
Copy link
Contributor

@sbrandhsn sbrandhsn commented Jan 18, 2024

Summary

Addresses and builds on top of #11407

Details and comments

With the removal of _index and _registers from Bit, Bits become opaque and can only be distinguished by their id. This breaks several test cases, requires a new de/serialization and a caching of ControlledGate.definition. These issues are addressed in this PR, leaving a potential refactoring of Layout for the future (see #11604).

@coveralls
Copy link

coveralls commented Jan 19, 2024

Pull Request Test Coverage Report for Build 7700646687

  • -9 of 70 (87.14%) changed or added relevant lines in 8 files are covered.
  • 25 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.03%) to 89.518%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/qpy/binary_io/circuits.py 11 15 73.33%
qiskit/passmanager/qiskit_dill.py 34 39 87.18%
Files with Coverage Reduction New Missed Lines %
qiskit/qpy/binary_io/circuits.py 3 91.82%
crates/qasm2/src/lex.rs 4 91.94%
crates/qasm2/src/parse.rs 18 96.69%
Totals Coverage Status
Change from base Build 7700276174: -0.03%
Covered Lines: 59527
Relevant Lines: 66497

💛 - Coveralls

@sbrandhsn sbrandhsn changed the title Removal of _index and _register fields from Bit Adapting Tests to New-Style Bits Jan 22, 2024
@sbrandhsn sbrandhsn marked this pull request as ready for review January 22, 2024 11:11
@sbrandhsn sbrandhsn requested review from nonhermitian and a team as code owners January 22, 2024 11:11
@qiskit-bot
Copy link
Collaborator

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

  • @enavarro51
  • @Qiskit/terra-core
  • @mtreinish
  • @nkanazawa1989

@sbrandhsn sbrandhsn changed the title Adapting Tests to New-Style Bits Removal of _index and _registers from Bit Jan 25, 2024
@sbrandhsn sbrandhsn requested review from 1ucian0 and kdk January 25, 2024 17:11
@1ucian0 1ucian0 self-assigned this Jan 25, 2024
@1ucian0 1ucian0 added this to the 1.0.0 milestone Jan 25, 2024
@mtreinish mtreinish modified the milestones: 1.0.0, 1.1.0 Jan 30, 2024
@1ucian0
Copy link
Member

1ucian0 commented Jul 24, 2024

@mtreinish The effects of this PR in QPY.. are breaking the QPY specification? If so, we could delay this to 2.0 or to increase the QPY version.

@mtreinish
Copy link
Member

There shouldn't be an issue for qpy here. The one place this will come up is probably in how we query the index and register in the serialization side. But if we're doing that we should be able to move to circuit.find_bit(). Qpy needs to be able to reason about the indices for bits and whether they're members of any registers. On the deserialization side there shouldn't be any changes needed because we shouldn't ever be setting these attributes manually.

@mtreinish mtreinish modified the milestones: 1.2.0, 2.0.0 Jul 25, 2024
@raynelfss
Copy link
Contributor

I'll be taking over these efforts from now on since this PR would be helpful for #13269.

@raynelfss raynelfss self-assigned this Oct 23, 2024
@raynelfss raynelfss force-pushed the remove-deprecated-bit-properties branch from 26834dd to 7f8cb05 Compare December 12, 2024 20:34
@1ucian0
Copy link
Member

1ucian0 commented Feb 10, 2025

Possible close this one as pointless now, since performance looks much better nowadays.

@raynelfss
Copy link
Contributor

Due to recent discussions and a change of perspective, we have decided to no longer pursue this approach. The PR is now closed! 🔨

@raynelfss raynelfss closed this Feb 13, 2025
@sbrandhsn
Copy link
Contributor Author

sbrandhsn commented Feb 13, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants