Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Deprecate QuantumInstance and Opflow #9176
Changes from 14 commits
07e5304
0f93f42
24dc678
a6c3b9e
62160e7
c3dde51
6e42e42
8924a73
050d8fb
b41b304
974654d
15fce2b
50e5954
11e74bd
cfb04f5
69c0119
f7835e3
fbd0438
96ccbd2
ad9249e
82efe0b
6f9e54d
153b9a0
ba9f614
6b6b2bc
585903b
40771b7
42ae937
1fa4aa2
da6a163
759a480
0d8f939
c07e69d
57c022b
efbe2c9
383db23
ccc6925
bd9090f
2613874
9c0a37f
541cb25
374209c
e9e6a4f
8b3136a
eaf7e92
72ee9b4
db0abd8
c8f974b
bef13d5
d31e095
b22e933
384636e
71e82e2
17ab34d
329b450
0ee225a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Adding a deprecation warning at the top should be enough, right? I considered reiterating this for each submodule listed.
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 only causes the deprecation to show up when reading docs. It does not issue a runtime warning. For that, you need to call
warnings.warn()
because I did not yet add a helper function for deprecating a module.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.
Since there are deprecation warnings in the code for each class adding another here in the code should not be needed I think. However having a note in the main opflow page about deprecations is highly desirable for when coming to this from the index. Yes with the new decorators it will be in each class docs too, but that is navigating away from this index page.
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.
Right, that is why I kept the warning in this page in particular. Once you go to the class docs, you should get the specific deprecation warning for that class.
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.
Not sure about the "deprecation effort" part of the message. I now realize I should probably also split this message along 2 lines, the string is too long.
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.
Yeah, seems fine to remove.
+1. I like using implicit string concatenation like this:
Note the space after the period in the first line.
It will not work to use
"""
because it messes up theadd_deprecation_to_docstring
helper function we call via these decorators.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.
That whole first sentence can likely go. And since it seems whatever text we end up with is in all of the warnings I do not know if there abetter way to facilitate this.
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.
You could always create a private helper function that wraps
@deprecate_func
to DRY things. Emphasis on private because we don't want this to be a long lasting public API; it's only to DRY this specific set of deprecations.I'm not sure if that messes up the stacktrace level though -- you would want to experiment with that to confirm.