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

Deprecate dense Operator Estimator observables #11056

Closed
wants to merge 1 commit into from
Closed

Deprecate dense Operator Estimator observables #11056

wants to merge 1 commit into from

Conversation

chriseclectic
Copy link
Member

Summary

This deprecates passing Operator and other dense BaseOperator subclasses as observables in Estimator.run. These were implicitly converted to SparsePauliOps by the Estimator, so now should be explicitly converted to SparsePauliOps by the user instead.

Details and comments

@qiskit-bot
Copy link
Collaborator

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

  • @ElePT
  • @Qiskit/terra-core
  • @ajavadia
  • @ikkoham
  • @levbishop
  • @t-imamichi
  • @woodsp-ibm

@chriseclectic chriseclectic added the Changelog: Deprecation Include in "Deprecated" section of changelog label Oct 19, 2023
@woodsp-ibm woodsp-ibm added the mod: primitives Related to the Primitives module label Oct 19, 2023
t-imamichi
t-imamichi previously approved these changes Oct 23, 2023
View reviewed changes
Copy link
Member

@t-imamichi t-imamichi left a comment

Choose a reason for hiding this comment

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

LGTM

@t-imamichi t-imamichi dismissed their stale review October 23, 2023 07:47

Found unit test errors

@t-imamichi
Copy link
Member

I made a PR to fix unit test errors. A unit test of gradient uses Operator as input.
chriseclectic#58

@chriseclectic
Deprecate dense Operator Estimator observables
fd1fafc 
This deprecates passing Operator and other dense `BaseOperator` subclasses as observables in `Estimator.run`. These were implicitly converted to SparsePauliOps by the Estimator, so now should be explicitly converted to `SparsePauliOps` by the user instead.
@chriseclectic chriseclectic requested a review from a team as a code owner January 9, 2024 19:13
ihincks
ihincks previously approved these changes Jan 9, 2024
View reviewed changes
@jakelishman jakelishman changed the base branch from main to stable/0.46 January 9, 2024 20:13
@jakelishman jakelishman dismissed ihincks’s stale review January 9, 2024 20:13

The base branch was changed.

@jakelishman jakelishman changed the base branch from stable/0.46 to main January 9, 2024 20:14
@jakelishman
Copy link
Member

New-deprecation PRs intended for 0.46 want to be targetted directly at stable/0.46, not main. I briefly changed the base branch, but it's from too new a commit for me to do without a force-push. The head commit should be rebased onto stable/0.46.

@chriseclectic chriseclectic changed the base branch from main to stable/0.46 January 9, 2024 21:21
@chriseclectic chriseclectic changed the base branch from stable/0.46 to main January 9, 2024 21:23
@chriseclectic
Copy link
Member Author

chriseclectic commented Jan 9, 2024 •
edited
Loading

@jakelishman It looks like there are other primitives deprecation's from previously merged PRs not included in 0.46 ( #11052 #11055). Do we need to merge into main, and then open a separate PR to backport these to stable/0.46 ?

@jakelishman
Copy link
Member

jakelishman commented Jan 9, 2024 •
edited
Loading

Luciano noticed #11055 and took / is taking care of it with #11520, but we didn't know about #11052. That needs applying to 0.46 too. We can leave the commits on main for now, provided another PR before 1.0 tidies them up. I can get the bot to prepare a port of #11052 to stable/0.46 (edit: #11532).

The difference to normal here is that the current backport branch is stable/0.45, and the development branch main is for 1.0. New deprecations for 0.46 aren't eligible to be made in the 0.45.x series, and they shouldn't be on main (the feature should be removed), which is why the PRs need making against the unreleased stable/0.46 directly; we're treating that as the development branch of 0.46.

1ucian0
1ucian0 reviewed Jan 10, 2024
View reviewed changes
qiskit/primitives/utils.py
@@ -59,6 +59,14 @@ def init_observable(observable: BaseOperator | str) -> SparsePauliOp:
if isinstance(observable, SparsePauliOp):
return observable
elif isinstance(observable, BaseOperator) and not isinstance(observable, BasePauli):
warnings.warn(
Copy link
Member

Choose a reason for hiding this comment

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

Considere adding a docstring note (similar to https://github.com/Qiskit/qiskit/pull/11520/files#diff-0094a250bbe85d99bff3279e6eaea98adaffcdb59e89773c76cb60be18a19675R157-R161).

1ucian0
1ucian0 reviewed Jan 10, 2024
View reviewed changes
qiskit/primitives/utils.py
Comment on lines +64 to +65
" observable arguments is deprecated as of Qiskit 0.46 and will be"
" in Qiskit 1.0. You should explicitly convert to a SparsePauli op using"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
" observable arguments is deprecated as of Qiskit 0.46 and will be"
" in Qiskit 1.0. You should explicitly convert to a SparsePauli op using"
" observable arguments is deprecated as of Qiskit 0.46 and will be removed"
" in Qiskit 1.0. You should explicitly convert to a SparsePauli op using"

1ucian0
1ucian0 requested changes Jan 10, 2024
View reviewed changes
Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

Thanks!

Just checking the style, consider adding a docstring note and a release note.

@chriseclectic
Copy link
Member Author

@jakelishman @1ucian0 since it would involve a full rebase to change the target branch I'll open a second PR for adding this to 0.46

@jakelishman
Copy link
Member

@Mergifyio backport stable/0.46

@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 10, 2024

backport stable/0.46

🟠 Waiting for conditions to match

  • merged [📌 backport requirement]

@jakelishman
Copy link
Member

ok, now the bot will just do it for you when this merges

@chriseclectic chriseclectic mentioned this pull request Jan 10, 2024
Merged
@chriseclectic
Copy link
Member Author

Replaced by #11535

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1ucian0 1ucian0 1ucian0 requested changes

@t-imamichi t-imamichi t-imamichi left review comments

@ihincks ihincks ihincks left review comments

@ikkoham ikkoham Awaiting requested review from ikkoham

@woodsp-ibm woodsp-ibm Awaiting requested review from woodsp-ibm

@ElePT ElePT Awaiting requested review from ElePT

@nonhermitian nonhermitian Awaiting requested review from nonhermitian

@eggerdj eggerdj Awaiting requested review from eggerdj

@wshanks wshanks Awaiting requested review from wshanks

@alexanderivrii alexanderivrii Awaiting requested review from alexanderivrii

@ShellyGarion ShellyGarion Awaiting requested review from ShellyGarion

@jyu00 jyu00 Awaiting requested review from jyu00

Assignees
No one assigned
Labels
Changelog: Deprecation Include in "Deprecated" section of changelog mod: primitives Related to the Primitives module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@chriseclectic @qiskit-bot @t-imamichi @jakelishman @1ucian0 @ihincks @woodsp-ibm