Deprecate QuantumInstance and Opflow by manoelmarques · Pull Request #9176 · Qiskit/qiskit

manoelmarques · 2022-11-22T04:52:12Z

Summary

Details and comments

These classes/functions outside the opflow module still use opflow as method argument or internal code:

qiskit.algorithms.time_evolvers.TimeEvolutionProblem -> argument + instance check
qiskit.primitives.BackendEstimator -> argument
qiskit.primitives.Estimator -> argument + instance check
qiskit.primitives.utils.init_observable ->argument + instance check
qiskit.primitives.base.BaseEstimator -> argument
qiskit.result.sampled.expval.sampled_expectation_value -> argument + instance check
~~qiskit.synthesis.evolution.QDrift~~ -> removed in this PR

qiskit-bot · 2022-11-22T04:52:16Z

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:

@Cryoris
@Qiskit/terra-core
@manoelmarques
@woodsp-ibm

coveralls · 2022-11-22T13:08:09Z

Pull Request Test Coverage Report for Build 4736561651

258 of 259 (99.61%) changed or added relevant lines in 71 files are covered.
286 unchanged lines in 24 files lost coverage.
Overall coverage increased (+0.1%) to 85.93%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/opflow/mixins/tensor.py	4	5	80.0%

Files with Coverage Reduction	New Missed Lines	%
qiskit/assembler/assemble_circuits.py	1	98.89%
qiskit/circuit/controlflow/if_else.py	1	97.69%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	95.37%
qiskit/transpiler/preset_passmanagers/level1.py	1	96.8%
crates/qasm2/src/lex.rs	2	91.9%
qiskit/qasm3/init.py	2	92.0%
qiskit/transpiler/passmanager_config.py	2	97.18%
qiskit/transpiler/preset_passmanagers/level0.py	2	94.12%
qiskit/transpiler/passes/utils/error.py	3	90.0%
qiskit/qasm3/ast.py	4	97.43%

Totals
Change from base Build 4721576551:	0.1%
Covered Lines:	71044
Relevant Lines:	82677

💛 - Coveralls

jakelishman

I'll be glad to see the back of QuantumInstance in particular. This is going to be one of the larger deprecations and transitions we have to navigate our users through, and I don't think we're offering enough support in this PR.

Some questions about how this deprecation is being done:

There are a lot of opflow imports remaining in qiskit.algorithms. What is happening to these? They should be migrated before the deprecation.
There are a lot of QuantumInstance-related methods and properties in algorithms. What is happening to these? Should these be part of the same deprecation?
Removing opflow and QuantumInstance is a massive change to the algorithms/applications interface. What transition guides do we have in place to manage this change? We should be promoting these heavily before any deprecations actually land in release Terra.

This is going to be a massive inconvenience for users, and we really need to make sure we're probably preparing them for the transition. All the warnings we're emitting need to be clear about what the alternative is, and give them links to how they can transition their code. We need those transition guides in place and being widely publicised, along with the reasons why, before we drop any warning messages on users.

General comments on the implementation here:

putting "Deprecated" into a docstring isn't the proper way to show deprecations - that's the .. deprecated:: directive.
documentation needs updating to show the new paths, not just to warnings.filterwarnings the messages away
It's not generally good to use catch_warnings in library code; it's slow, clunky, and often hides true warnings. Instead, the stacklevel arguments should be set correctly such that Qiskit library code should be blamed for the warning where appropriate, which prevents DeprecationWarning from being shown to users with default filters.
in all deprecate_function calls, we should be pointing users to what they should be using instead, preferably via a complete migration guide.
the testing of the warnings here is not sufficient. We cannot suppress warnings generated by algorithms and other Terra places because of opflow use - we need to know about them, and fix them before the warnings are emitted. If we can't do that, how are we expecting users to do it? If there are undeprecated public entry points in algorithms and elsewhere that should warn on certain inputs, then they should have tests asserting that they do.

Other places to consider:

qiskit.utils.backend_utils feels very QuantumInstance-y. Can we deprecate that now?
qiskit.utils.mitigation.fitters refers to QuantumInstace. Do we need to do something about this?

…arnings

Restore opflow test case Restore estimator test Restore unchanged tests

This reverts commit 9c0a37f.

Review is long since stale

woodsp-ibm

Seems we are finally there with this. It all looks good to me with the code and the API ref docs from it. Thanks @ElePT for picking thus up and seeing it through to the end, and thanks to @manoelmarques for the initial effort this was based on.

mtreinish

This LGTM thanks for all the hard work in doing this!

I did catch one typo inline but we can fix this after this merges, I'd rather enqueue this now rather than waiting to fix a single character out of ~4k changed lines.

mtreinish · 2023-04-18T22:34:06Z

+)
 def make_immutable(obj):
-    """Delete the __setattr__ property to make the object mostly immutable."""
+    r"""Deprecate\: Delete the __setattr__ property to make the object mostly immutable."""


Suggested change

r"""Deprecate\: Delete the __setattr__ property to make the object mostly immutable."""

r"""Deprecated\: Delete the __setattr__ property to make the object mostly immutable."""

This method is not used outside or operator globals. It could have been private rather than seeming to be public. Either way the intent was that this function was more an internal function in opflow and it is not linked in the API ref. so the spelling without a d at the end will only ever be seen if you look at the source code - hence it could just as well be left as it.

dongreenberg · 2023-04-20T00:20:57Z

Oh man can't believe it lasted this long! I too am glad to see the quantum instance behind us, and glad the quantum_info is now far enough along that we don't need duplicate ops and states for the algos. Amazing work on the migration guides! Crazy comprehensive.

manoelmarques added Changelog: Deprecated Add a "Deprecated" entry in the GitHub Release changelog. mod: opflow labels Nov 22, 2022

manoelmarques self-assigned this Nov 22, 2022

manoelmarques requested review from a team, ikkoham and woodsp-ibm as code owners November 22, 2022 04:52

manoelmarques force-pushed the opflow branch from bc7287d to 4a07822 Compare November 22, 2022 12:38

jakelishman previously requested changes Nov 22, 2022

View reviewed changes

manoelmarques marked this pull request as draft November 22, 2022 13:25

manoelmarques changed the title ~~Deprecate QuantumInstance and Opflow~~ [WIP] Deprecate QuantumInstance and Opflow Nov 24, 2022

manoelmarques force-pushed the opflow branch 2 times, most recently from 6b39927 to fb44b40 Compare December 12, 2022 18:27

woodsp-ibm mentioned this pull request Dec 13, 2022

Use opflow operators in dynamics qiskit-community/qiskit-dynamics#167

Closed

manoelmarques force-pushed the opflow branch 15 times, most recently from c33ea07 to e1c28d2 Compare January 8, 2023 01:45

ElePT added 6 commits April 17, 2023 12:42

Merge from main, apply algo deprecation, use assertWarns instead of w…

2613874

…arnings

Refactor gradients test warnings

9c0a37f

Remove warning filter

541cb25

Refactor rest of tests to assert warnings

374209c

Fix formatting

e9e6a4f

Restore unchanged unit tests

8b3136a

Restore opflow test case Restore estimator test Restore unchanged tests

ElePT force-pushed the opflow branch from 27df4b1 to 8b3136a Compare April 17, 2023 12:43

ElePT added 4 commits April 17, 2023 15:24

Go back to old opflow test setup

eaf7e92

Revert "Refactor gradients test warnings", only keep qi assertWarns

72ee9b4

This reverts commit 9c0a37f.

Restore opflow unittests

db0abd8

Fix conflict from Qiskit#9322

c8f974b

ElePT added 6 commits April 17, 2023 17:15

Fix tests

bef13d5

Fix lint

d31e095

Fix qaoa repeated test

b22e933

Add module deprecation warning

384636e

Divide message in multiple lines

71e82e2

Fix lint

17ab34d

woodsp-ibm reviewed Apr 17, 2023

View reviewed changes

Comment thread qiskit/opflow/__init__.py Outdated

:mod:: -> :mod:

329b450

woodsp-ibm mentioned this pull request Aug 18, 2023

Operator tutorials and opflow deprecation #10666

Closed

Deprecate Z2 symmetries

0ee225a

woodsp-ibm approved these changes Apr 18, 2023

View reviewed changes

mtreinish approved these changes Apr 18, 2023

View reviewed changes

mtreinish added this pull request to the merge queue Apr 18, 2023

Merged via the queue into Qiskit:main with commit 5128c67 Apr 18, 2023

t-imamichi mentioned this pull request Apr 20, 2023

Suppress Opflow deprecation warnings in test_qpy.py #9998

Merged

woodsp-ibm mentioned this pull request Apr 20, 2023

Fix lint after opflow deprecation qiskit-community/qiskit-nature#1135

Merged

	r"""Deprecate\: Delete the __setattr__ property to make the object mostly immutable."""
	r"""Deprecated\: Delete the __setattr__ property to make the object mostly immutable."""

Conversation

manoelmarques commented Nov 22, 2022 • edited by ElePT Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

qiskit-bot commented Nov 22, 2022

Uh oh!

coveralls commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 4736561651

💛 - Coveralls

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

woodsp-ibm left a comment

Choose a reason for hiding this comment

Uh oh!

mtreinish left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtreinish Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

woodsp-ibm Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

dongreenberg commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

manoelmarques commented Nov 22, 2022 •

edited by ElePT

Loading

coveralls commented Nov 22, 2022 •

edited

Loading

mtreinish left a comment •

edited

Loading