-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Deprecate QuantumInstance and Opflow #9176
Conversation
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:
|
bc7287d
to
4a07822
Compare
Pull Request Test Coverage Report for Build 4736561651
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inqiskit.algorithms
. What is happening to these? They should be migrated before the deprecation. - There are a lot of
QuantumInstance
-related methods and properties inalgorithms
. What is happening to these? Should these be part of the same deprecation? - Removing
opflow
andQuantumInstance
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, thestacklevel
arguments should be set correctly such that Qiskit library code should be blamed for the warning where appropriate, which preventsDeprecationWarning
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 ofopflow
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 inalgorithms
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 veryQuantumInstance
-y. Can we deprecate that now?qiskit.utils.mitigation.fitters
refers toQuantumInstace
. Do we need to do something about this?
qiskit/algorithms/linear_solvers/observables/absolute_average.py
Outdated
Show resolved
Hide resolved
qiskit/algorithms/linear_solvers/observables/matrix_functional.py
Outdated
Show resolved
Hide resolved
6b39927
to
fb44b40
Compare
c33ea07
to
e1c28d2
Compare
Restore opflow test case Restore estimator test Restore unchanged tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r"""Deprecate\: Delete the __setattr__ property to make the object mostly immutable.""" | |
r"""Deprecated\: Delete the __setattr__ property to make the object mostly immutable.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
* Deprecate QuantumInstance and Opflow * Add .. deprecated:: to init modules * Add link to url guide * Remove internal use of opflow * Fix black * Discard old changes in deprecation.py * Update decorators * Remove from docstring * Update sphinx deprecations, fix lint, fix tests * Remove hanging Deprecated in docstring * Remove old Deprecation messages * Revert "Remove old Deprecation messages" This reverts commit 6924130. * Revert "Remove hanging Deprecated in docstring" This reverts commit cfb04f5. * Revert "Remove from docstring" This reverts commit 50e5954. * Change Deprecation to Deprecated * Remove catch warnings in main code * Fix missing decorators * Fix tests * Shorten message * Fix black * Shorten qi message * Remove checks for QDrift's internal use of opflow (changed in this PR) * Add warning catchers to unit tests * Remove opflow from qaoa ansatz tests * Remove opflow from non-related tests * Restore some opflow tests for completion * Fix black * Remove catch warnings * Refactor adaptVQE tests, remove opflow from observables evaluator * Fix tests, lint Fix tests, lint Fix CI failures Fix tests CI Fix unit tests Fix CI again Fix lint Restore vqd test Remove unused import * Delete qobj test to fix CI until Qiskit#9322 is merged * Refactor gradients test warnings * Remove warning filter * Refactor rest of tests to assert warnings * Fix formatting * Restore unchanged unit tests Restore opflow test case Restore estimator test Restore unchanged tests * Go back to old opflow test setup * Revert "Refactor gradients test warnings", only keep qi assertWarns This reverts commit 9c0a37f. * Restore opflow unittests * Fix tests * Fix lint * Fix qaoa repeated test * Add module deprecation warning * Divide message in multiple lines * Fix lint * :mod:: -> :mod: * Deprecate Z2 symmetries --------- Co-authored-by: ElePT <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: woodsp-ibm <[email protected]>
* Deprecate QuantumInstance and Opflow * Add .. deprecated:: to init modules * Add link to url guide * Remove internal use of opflow * Fix black * Discard old changes in deprecation.py * Update decorators * Remove from docstring * Update sphinx deprecations, fix lint, fix tests * Remove hanging Deprecated in docstring * Remove old Deprecation messages * Revert "Remove old Deprecation messages" This reverts commit 6924130. * Revert "Remove hanging Deprecated in docstring" This reverts commit cfb04f5. * Revert "Remove from docstring" This reverts commit 50e5954. * Change Deprecation to Deprecated * Remove catch warnings in main code * Fix missing decorators * Fix tests * Shorten message * Fix black * Shorten qi message * Remove checks for QDrift's internal use of opflow (changed in this PR) * Add warning catchers to unit tests * Remove opflow from qaoa ansatz tests * Remove opflow from non-related tests * Restore some opflow tests for completion * Fix black * Remove catch warnings * Refactor adaptVQE tests, remove opflow from observables evaluator * Fix tests, lint Fix tests, lint Fix CI failures Fix tests CI Fix unit tests Fix CI again Fix lint Restore vqd test Remove unused import * Delete qobj test to fix CI until Qiskit#9322 is merged * Refactor gradients test warnings * Remove warning filter * Refactor rest of tests to assert warnings * Fix formatting * Restore unchanged unit tests Restore opflow test case Restore estimator test Restore unchanged tests * Go back to old opflow test setup * Revert "Refactor gradients test warnings", only keep qi assertWarns This reverts commit 9c0a37f. * Restore opflow unittests * Fix tests * Fix lint * Fix qaoa repeated test * Add module deprecation warning * Divide message in multiple lines * Fix lint * :mod:: -> :mod: * Deprecate Z2 symmetries --------- Co-authored-by: ElePT <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: woodsp-ibm <[email protected]>
* Deprecate QuantumInstance and Opflow * Add .. deprecated:: to init modules * Add link to url guide * Remove internal use of opflow * Fix black * Discard old changes in deprecation.py * Update decorators * Remove from docstring * Update sphinx deprecations, fix lint, fix tests * Remove hanging Deprecated in docstring * Remove old Deprecation messages * Revert "Remove old Deprecation messages" This reverts commit 6924130e12ffdfde971ff1e25434ee84bf983482. * Revert "Remove hanging Deprecated in docstring" This reverts commit cfb04f50e6a30a33955b57be4ab5bbdb9035923c. * Revert "Remove from docstring" This reverts commit 50e5954ecd4a729e7101e8ea4bf493240ae8b59e. * Change Deprecation to Deprecated * Remove catch warnings in main code * Fix missing decorators * Fix tests * Shorten message * Fix black * Shorten qi message * Remove checks for QDrift's internal use of opflow (changed in this PR) * Add warning catchers to unit tests * Remove opflow from qaoa ansatz tests * Remove opflow from non-related tests * Restore some opflow tests for completion * Fix black * Remove catch warnings * Refactor adaptVQE tests, remove opflow from observables evaluator * Fix tests, lint Fix tests, lint Fix CI failures Fix tests CI Fix unit tests Fix CI again Fix lint Restore vqd test Remove unused import * Delete qobj test to fix CI until Qiskit/qiskit#9322 is merged * Refactor gradients test warnings * Remove warning filter * Refactor rest of tests to assert warnings * Fix formatting * Restore unchanged unit tests Restore opflow test case Restore estimator test Restore unchanged tests * Go back to old opflow test setup * Revert "Refactor gradients test warnings", only keep qi assertWarns This reverts commit 9c0a37f3bc3068a9b1ccd122298cfefe556d4135. * Restore opflow unittests * Fix tests * Fix lint * Fix qaoa repeated test * Add module deprecation warning * Divide message in multiple lines * Fix lint * :mod:: -> :mod: * Deprecate Z2 symmetries --------- Co-authored-by: ElePT <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: woodsp-ibm <[email protected]>
* Deprecate QuantumInstance and Opflow * Add .. deprecated:: to init modules * Add link to url guide * Remove internal use of opflow * Fix black * Discard old changes in deprecation.py * Update decorators * Remove from docstring * Update sphinx deprecations, fix lint, fix tests * Remove hanging Deprecated in docstring * Remove old Deprecation messages * Revert "Remove old Deprecation messages" This reverts commit 6924130e12ffdfde971ff1e25434ee84bf983482. * Revert "Remove hanging Deprecated in docstring" This reverts commit cfb04f50e6a30a33955b57be4ab5bbdb9035923c. * Revert "Remove from docstring" This reverts commit 50e5954ecd4a729e7101e8ea4bf493240ae8b59e. * Change Deprecation to Deprecated * Remove catch warnings in main code * Fix missing decorators * Fix tests * Shorten message * Fix black * Shorten qi message * Remove checks for QDrift's internal use of opflow (changed in this PR) * Add warning catchers to unit tests * Remove opflow from qaoa ansatz tests * Remove opflow from non-related tests * Restore some opflow tests for completion * Fix black * Remove catch warnings * Refactor adaptVQE tests, remove opflow from observables evaluator * Fix tests, lint Fix tests, lint Fix CI failures Fix tests CI Fix unit tests Fix CI again Fix lint Restore vqd test Remove unused import * Delete qobj test to fix CI until Qiskit/qiskit#9322 is merged * Refactor gradients test warnings * Remove warning filter * Refactor rest of tests to assert warnings * Fix formatting * Restore unchanged unit tests Restore opflow test case Restore estimator test Restore unchanged tests * Go back to old opflow test setup * Revert "Refactor gradients test warnings", only keep qi assertWarns This reverts commit 9c0a37f3bc3068a9b1ccd122298cfefe556d4135. * Restore opflow unittests * Fix tests * Fix lint * Fix qaoa repeated test * Add module deprecation warning * Divide message in multiple lines * Fix lint * :mod:: -> :mod: * Deprecate Z2 symmetries --------- Co-authored-by: ElePT <[email protected]> Co-authored-by: Elena Peña Tapia <[email protected]> Co-authored-by: woodsp-ibm <[email protected]>
Summary
Details and comments
These classes/functions outside the opflow module still use opflow as method argument or internal code:
qiskit.synthesis.evolution.QDrift-> removed in this PR