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

Remove opflow classes from primitives #11209

Closed
wants to merge 1 commit into from

Conversation

mberna
Copy link

@mberna mberna commented Nov 7, 2023

Summary

PauliSumOp class has been deprecated since 0.24.0. This PR removes PauliSumOp support from the primitives.

Details and comments

Updated the tests accordingly.

@mberna mberna requested review from ikkoham, t-imamichi and a team as code owners November 7, 2023 04:18
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Nov 7, 2023
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

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

@CLAassistant
Copy link

CLAassistant commented Nov 7, 2023

CLA assistant check
All committers have signed the CLA.

@t-imamichi
Copy link
Member

t-imamichi commented Nov 7, 2023

Opflow is being removed by #11111. It contains the same changes as this PR.

@t-imamichi t-imamichi added mod: opflow Related to the Opflow module mod: primitives Related to the Primitives module labels Nov 7, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 6779944663

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 86.949%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 91.16%
Totals Coverage Status
Change from base Build 6774672560: 0.02%
Covered Lines: 74344
Relevant Lines: 85503

💛 - Coveralls

@@ -350,7 +349,7 @@ class TestObservableValidation(QiskitTestCase):
("IXYZ", (SparsePauliOp("IXYZ"),)),
(Pauli("IXYZ"), (SparsePauliOp("IXYZ"),)),
(SparsePauliOp("IXYZ"), (SparsePauliOp("IXYZ"),)),
(PauliSumOp(SparsePauliOp("IXYZ")), (SparsePauliOp("IXYZ"),)),
(SparsePauliOp(SparsePauliOp("IXYZ")), (SparsePauliOp("IXYZ"),)),
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to test SparsePauliOp(SparsePauliOp(...)). The test of SparsePauliOp(...) is enough.

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Thanks @mberna, as @t-imamichi mentioned we were planning to do this as part of #11111, which should be ready as soon as the algorithms removal is merged. Is there any particular reason why this should be done earlier?

@mberna
Copy link
Author

mberna commented Nov 9, 2023

Since #11111 is still a draft and also marked as on-hold until #11086 is merged, would it be possible to merge this PR first? This will enable the removal of PauliSumOp from https://github.com/Qiskit/qiskit-ibm-runtime

@jakelishman
Copy link
Member

Even if we merged this right now, it won't be in even a pre-release version of Qiskit until a couple of days before Summit, so I'm not certain that it'll accelerate your timelines.

#11111 is only in draft / on hold as a marker that it's not the first step in the process of safe removal of opflow. It's expected to be just about ready to go. We're working through that process, but we need to take care that it's done safely, so we don't inadvertently break seemingly unrelated code that other Summit demos might be relying on - for example, the recent release of Aer 0.13 showed that there's still some surprising dependence on some old Aqua-inherited / opflow-related code in qiskit.utils that we're also unravelling.

The full timeline of things that happen to remove opflow is this:

Of these, the removal of qiskit.algorithms is all ready to go as soon as we can be sure that all our downstream projects are safe with the migration (while it's Nature we explicitly test against, this is a proxy that all uses can be replaced). After that, the opflow-removal PR (#11111) is pretty immediately ready to go, and that complete covers this PR.

This PR shouldn't merge before opflow is removed, because until it actually is, it would be a backwards incompatible change to remove the inputs from the primitives. The only reason we're allowed to remove the input from the primitives at all is because it wouldn't be possible to construct an opflow object; until that time, it must remain valid as an input. This PR is also currently missing several of the references to opflow objects that exist within the primitives-validation code.

@ElePT
Copy link
Contributor

ElePT commented Nov 24, 2023

#11111 Has finally been merged 🎉 , so this PR is no longer necessary. Thanks for your patience :)

@ElePT ElePT closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: opflow Related to the Opflow module mod: primitives Related to the Primitives module
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants