-
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
Remove runtime program, quantum instance, and opflow #542
Conversation
4745eaa
to
e30955f
Compare
e30955f
to
b3bbd53
Compare
Pull Request Test Coverage Report for Build 6032830758
💛 - Coveralls |
Yes, the intent is to cease use of deprecated function like that so supporting just SparsePauliOp is all that should be done. In terms of other deprecated code in Opt, I did a quick search and this was all that came up for me. We should remove this too - in this case its time is up anyway (could have been removed last release) qiskit-optimization/qiskit_optimization/applications/graph_optimization_application.py Lines 83 to 89 in e2855f2
|
releasenotes/notes/remove-quantum-instance-and-runtime-2c00393c0de1b9a9.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/remove-quantum-instance-and-runtime-2c00393c0de1b9a9.yaml
Outdated
Show resolved
Hide resolved
b06b2f2
to
c170f10
Compare
I removed opflow and updated tutorials and reno following Steve's feedback. |
if self._sampler is None: | ||
raise ValueError("A sampler must be provided.") |
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 is just a comment on this. Given that a Sampler is now the only option with the removal of QI we could have defaulted the Sampler to the qiskit.primitives one if it was None in the constructor (and noted this for the sampler param there). That way the default value does not lead to an error - which seems strange. I get why it was done having to support both QI and Sampler. Since that is more of an improvement, and the GroverOptimizer was being looked at to be improved anyway, I think we can live with it like this, so as I said 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.
Yes, we can make Sampler mandatory. But we need to change the argument order (need to move sampler
first or second). Do you think whether we can change the argument order of GroverOptimizer.__init__
?
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 was not talking about that much of a change. Leave it default to None and in the constructor do something like this
if sampler is None:
sampler = Sampler() # qiskit.primitives.Sampler
se we default it to the reference Sampler, and say we do this for the sampler param docstring. I think for the GroverOptimizer there had been some work done, not sure how far it went, to look at the issue raised some while back which was to have this use Grover rather than contain similar code. I think I would leave any more radical change until such a time as that might be done.
As I mentioned I am fine with leaving it like it is - its not that the GroverOptimizer is used much. Its not so great having an invalid default value as such but it is what it is due to this QI migration. The internal creating an instance of the default Sampler, if it were None, I thought more a simple improvement that might be done.
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 got it. I leave it as is. I will then work on #537 after merging this PR tomorrow.
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 good to me. I left a comment above but its just that a comment - I don't see it blocking this.
Summary
This PR removes runtime clients, which have been already removed at IBM Quantum Platform, and
QuantumInstance
-based algorithms.QuantumInstance
-based legacy algorithmsDetails and comments
It still uses opflow-basedPauliSumOp
somewhere for the compatibility. E.g.,to_ising
function to returnPauliSumOp
with the default option (optionopflow=False
returnsSparsePauliOp
). Should I remove them all as well?→ Opflow is also removed.