-
Notifications
You must be signed in to change notification settings - Fork 140
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
Replace qiskit.algorithms with qiskit_algorithms #537
Conversation
|
Pull Request Test Coverage Report for Build 5949370263
💛 - Coveralls |
Yes, I added qiskit-algorithms in requirements.txt. OK. I will switch to |
Should be very soon now. |
It's ready to review because qiskit-algorithms 0.2.0 was released on Aug 24. |
@t-imamichi My take is that I think the logic ought to be able to take a qiskit.algorithms instance or a qiskit_algorithms one. The instance checks there I think will prevent the former. That may end up being a tradeoff in what sort of upfront checks can be made and how much we want to switch to just using qiskit_algorithms and no longer refer to qiskit.algorithms at this point. Did you see the post in the thread in the internal algorithms related discussion channel - where you mentioned this PR it was a response to that - it includes some thoughts about this and more. |
@woodsp-ibm Thanks. I tried to remove all imports of |
# Conflicts: # docs/tutorials/02_converters_for_quadratic_programs.ipynb # qiskit_optimization/algorithms/grover_optimizer.py # qiskit_optimization/algorithms/minimum_eigen_optimizer.py # qiskit_optimization/algorithms/warm_start_qaoa_optimizer.py # requirements.txt # test/algorithms/test_grover_optimizer.py # test/algorithms/test_min_eigen_optimizer.py
if not hasattr(eigen_result, "eigenstate"): | ||
raise QiskitOptimizationError( | ||
"MinimumEigenOptimizer does not support this minimum eigensolver " | ||
f"{type(self._min_eigen_solver)}. " | ||
"You can use qiskit_algorithms.SamplingMinimumEigensolver instead." | ||
) |
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.
Instead of early type check of SamplingMinimumEigensolver or NumPyMinimumEigensolver, I added a result check when solve is invoked.
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.
It will certainly result in a more friendly error message that is easier to understand. This is of course detected though after its run compute_minimum_eigenvalue which may take a lot of time/resources that ends up being wasted. An IDE via the typehinting should flag VQE (which I think we we most concerned about in 0.5 since in the past you could use this) as not being valid to be used for this algorithm. I think its ok - we can see how it goes - one suggestion I had made was to get the class name string from the solver instance and compare that up front to see if it was one of the primitive based VQE's we know about ie - that being just a string check does not require anything to be imported. But we could always add this later if users are still tripping up on VQE - since 0.5 has been out a while hopefully users are familiar enough now. And its quite clear now in the docs what is supported...
qiskit_optimization/__init__.py
Outdated
`Grover Adaptive Search <https://arxiv.org/abs/quant-ph/9607014>`_ | ||
(:class:`~algorithms.GroverOptimizer`), leveraging | ||
fundamental :mod:`~qiskit.algorithms.minimum_eigensolver` provided by Qiskit Terra. | ||
fundamental :mod:`~qiskit_algorithms.minimum_eigensolvers` provided by |
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.
With qiskit_algorithms this will not form a link as it will not resolve to any documentation since the minimum_eigensolvers module is not documented as it was before (before it was not a link either since it lacked the s
at the end). I think its ok.
The Qiskit Algorithms link on the next line I think would be better to the published docs though so you go from docs to docs ie. use https://qiskit.org/ecosystem/algorithms/
And for the module above if you preferred it to be a link then it could be changed to make it a link to where the Minimum Eigensolvers
are documented ie this https://qiskit.org/ecosystem/algorithms/apidocs/qiskit_algorithms.html#minimum-eigensolvers
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.
Thanks. I updated it to mention minimum eigensolvers with https://qiskit.org/ecosystem/algorithms/apidocs/qiskit_algorithms.html#minimum-eigensolvers instead of ~qiskit_algorithms.minimum_eigensolvers
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.
Thx, you did not push the change yet then?
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.
Sorry. I forgot to push it. 10d057b
prelude: > | ||
Qiskit Optimization 0.6 switches from ``qiskit.algorithms`` of Qiskit Terra | ||
to `Qiskit Algorithms <https://github.com/qiskit-community/qiskit-algorithms>`_. | ||
Qiskit Optimization 0.6 drops supports of the former algorithms 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.
Although support for the qiskit.algorithms primitive based ones is dropped, for the moment since they are compatible with what was migrated to qiskit_algorithms they should still work so any users code should still work as they transition/upgrade it - right.
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'm wondering whether I have to mention qiskit.algorithms is not officially supported by qiskit-optimization but it works actually.
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.
The intent was really to have it work so users had a easier transition. Code that they had working based on primitives should still work with this new version. Then they can simply switch the code to qiskit_algorithms and it should work just like before.
@@ -40,6 +41,7 @@ | |||
class TestGroverOptimizer(QiskitOptimizationTestCase): | |||
"""GroverOptimizer tests.""" | |||
|
|||
@unittest.skipUnless(optionals.HAS_AER, "qiskit-aer is required to run this test") |
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.
At some point, since the reference Sampler is probably fast enough, we could probably change tests like these so we do not require Aer any longer and this test no longer is an optional one - though CI of course will install Aer. I would leave things as they are now. Its more just a comment.
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.
6ec2a94
to
1418093
Compare
@t-imamichi When can we expect this to be released? |
Summary
Replace
qiskit.algorithms
withqiskit_algorithms
exceptQAOAClient
andVQEClient
because they are deprecated.This PR requires qiskit-algorithms>=0.2.0 to switch to
qiskit_algorithms.utils.algorithm_globals
.Details and comments