-
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
Add deprecation warning to algs module #10406
Conversation
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.
Looks good! Thanks.
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Pull Request Test Coverage Report for Build 5611202474
💛 - Coveralls |
One or more of the the following people are requested to review this:
|
I had mentioned the docs, migration guides in that we might want to add something there. What about release notes - again something can be added after but I imagine this ought to get noted at that level. |
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 added a release note in: e1a1c62 and it LGTM now. Feel free to change any of the text in it, I struggled a bit with what to say in there and I'm still not super satisfied with the exact text (but that's probably my bias against the decision to migrate it out of terra leaking through) we can also always tweak the exact release note contents until the final release. I'll hold off on adding to the merge queue until @woodsp-ibm and/or @Cryoris think it's ready to go
It LGTM - the only comment/text I might make/add is that if users are still using the old deprecated algorithms/QuantumInstance they need to first migrate to the primitive based algos and then switch. We might want to point to the algorithm migration guide in the release note along with such a comment to aid them in this regard. If they are already using the primitive based algos then they should be good to use qiskit_algorithms and it should just be that simple import change. |
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
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 and I've verified from qiskit import algorithms
raises the warnings as expected so I think we're good to go here and we can concentrate the efforts on getting the qiskit-algorithms
package ready.
Summary
This PR shows a deprecation message proposal for the migration of algorithms to a separate community repo (see Qiskit/RFCs#44). Given that there is no module-level decorator, the message is not really standardized, but I believe that wording the message right is particularly important in this case, so please don't hesitate to comment/give suggestions (@1ucian0, @Eric-Arellano)
Details and comments
This PR is blocked by the actual creation of the new repository.