Skip to content

Deprecate Bit.index and Bit.register#6069

Merged
mergify[bot] merged 5 commits into
Qiskit:masterfrom
kdk:quantumcircuit-optional-registers-deprecate-index-register-access
Apr 1, 2021
Merged

Deprecate Bit.index and Bit.register#6069
mergify[bot] merged 5 commits into
Qiskit:masterfrom
kdk:quantumcircuit-optional-registers-deprecate-index-register-access

Conversation

@kdk
Copy link
Copy Markdown
Member

@kdk kdk commented Mar 22, 2021

Summary

Deprecate references from Bit objects to their containing Register.

Details and comments

on hold for #5519 .

@kdk kdk added Changelog: Deprecated Add a "Deprecated" entry in the GitHub Release changelog. optional-registers labels Mar 22, 2021
@kdk kdk added this to the 0.17 milestone Mar 22, 2021
@kdk kdk requested a review from a team as a code owner March 22, 2021 22:32
@kdk kdk added the on hold Can not fix yet label Mar 22, 2021
@kdk kdk self-assigned this Mar 23, 2021
@kdk kdk force-pushed the quantumcircuit-optional-registers-deprecate-index-register-access branch from e53c608 to 06bf861 Compare March 31, 2021 13:39
@mtreinish mtreinish removed the on hold Can not fix yet label Mar 31, 2021
Copy link
Copy Markdown
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Overall LGTM 2 questions inline, the one thing I'm not clear on is whether we're just deprecating Qubit.index or this is also deprecating the registers pieces in the constructor. Looking at the code and the release notes it's only about the attributes/properties, but this updates the docstring for Clbit and Qubit to say deprecated.

Comment thread qiskit/circuit/classicalregister.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we deprecating the constructor or just the access?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch, this is left over from an earlier iteration. Right now just access, constructor will be done when we deprecate implicit register creation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reverted in 17406ac .

Comment thread qiskit/visualization/gate_map.py Outdated
Comment on lines 361 to 363
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't remember how we handled registerless bits in the layout, but won't this potentially miss a virtual bit if it's not in a register?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct, fixed in afdffa3 .

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
via the :meth:`~qiskit.circuit.Qubit.register` or :meth:`~qiskit.circuit.Qubit.index`
via the :attr:`~qiskit.circuit.Qubit.register` or :attr:`~qiskit.circuit.Qubit.index`

Only merge this if there are no other changes (to avoid ci churn), if not I can fix this in #6075

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in afdffa3 .

@kdk kdk force-pushed the quantumcircuit-optional-registers-deprecate-index-register-access branch from 06bf861 to afdffa3 Compare March 31, 2021 16:13
mtreinish
mtreinish previously approved these changes Mar 31, 2021
Copy link
Copy Markdown
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, it'd be good to have some gate_map visualization tests without a register but that can come in a follow up (especially since those tests need to be refactored anyway)

@kdk kdk force-pushed the quantumcircuit-optional-registers-deprecate-index-register-access branch from 081e077 to d7ccc5a Compare March 31, 2021 21:01
mtreinish
mtreinish previously approved these changes Mar 31, 2021
@kdk kdk force-pushed the quantumcircuit-optional-registers-deprecate-index-register-access branch from d7ccc5a to 7b18bb7 Compare April 1, 2021 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: Deprecated Add a "Deprecated" entry in the GitHub Release changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants