Remove Opflow and QI #17
Conversation
There was a problem hiding this comment.
It's a solid idea not to bring over-deprecated code into this new repo because it continues to exist in Terra. That indeed makes this repo easier to work with.
You only will want to consider improving the deprecation messages here and in Terra to clarify that for opflow, you should keep using Terra rather than qiskit_algorithms. As in, runtime warnings and our documentation.
But, it is an awkward user experience that they're going to get one deprecation message saying "use qiskit_algorithms!" and another that says "opflow is deprecated, and don't use qiskit_algorithms". So, it depends on how important it is to avoid that confusing UX.
Thanks @Eric-Arellano!!! I think that if we improve the terra deprecation message to specify that if you absolutely want to keep using the old code you should not migrate to the new repo, and the releasenote in #19, it is hopefully clear enough for users. I think it is the best way to go about it. |
|
If people want to stick with the deprecated code, or use it as they see an example out there these will all be Terra and they can stay with that as long as the code exists. To move here, they need to get off the deprecated code. Ideally they should do this as soon as its convenient for them as the code will be removed and no longer supported later this year. I see no reason to duplicate deprecated code here, indeed every reason not too. It was just expedient to copy the whole algos over, then prune things out. Some logic has already been removed right, the QI based (minimum)eigensolvers where it was all or nothing in terms of the code. What remains is just the algorithms like Grover which supported both and an internal switch one way or another so removal is a little bit more delicate surgery. Having some logic here supporting opflow/QI and some already gone will be rather confusing. This should be a clean start for primitive based algos - but is one that also alleviates moving over to it as its the same primitive based algos as in Terra just a slightly different import. But you have to have migrated to using that in Terra or migrate directly here with the caveat that its the same deal but with the extra import change too. In regards of Eric's comment |
To clarify, I mean the holistic experience of deprecation messages, including considering tweaking messages in Terra itself. |
Yes, the entire "messaging" which includes what we say in release notes. maybe augmenting the existing algorithm migration guide with info around this additional move etc I only picked on that bit since it seemed to be talking about deprecation warnings/messages here, and I think we will have none being emitted from the code. Now we have text in a release note trying to explain things - I just updated it in #13 to fix it for CI and at the same time I tried to improve the content/format, and issue #7 was created with an intent to add some stuff to the readme here that could also inform what happening with the algorithms. So addressing it from a holistic view I agree. |
…gorithms into remove-opflow-qi
| @@ -311,7 +310,8 @@ def estimate(self, estimation_problem: EstimationProblem) -> "AmplitudeEstimatio | |||
| "The state_preparation property of the estimation problem must be set." | |||
| ) | |||
| if self._sampler is None: | |||
| raise ValueError("A sampler must be provided.") | |||
| warnings.warn("No sampler provided, defaulting to Sampler from qiskit.primitives") | |||
There was a problem hiding this comment.
I would have more likely done this in the sampler setter - let it take None and if None set to the reference one. Document in the constructor/setter that None will use the reference sampler - set it in the contructor via the setter. And then no need for any warning. This is what ML does for QNNs taking a Sampler etc. This will work though and if you want to go with this for now...
Summary
This PR fixes #15. I think that this is a helpful removal in terms of code maintenance and "self-containedness" of the qiskit-algorithms repo, however, I am not 100% sure about how strict we should be with Qiskit's deprecation policy, which states that the code should stay for 2 releases after deprecation. I am not too worried because the old code is still in terra, but I wonder if it is best to keep this PR on hold until the terra code is removed.
Also fixes #11.
Details and comments
There are a couple of unit tests that fail, I wanted to put the PR out there first, and then ask for help to fix these specific algorithms, as I am not a particular expert in them (I suspect that I butchered them a bit). Namely:
test.time_evolvers.test_trotter_qrte-> deprecation warning handling onlytest.test_phase_estimator-> deprecation warning handling onlytest.test_amplitude_estimatorstest.optimizers.test_spsatest.optimizers.test_optimizers(qnspsa test)test.optimizers.test_gradient_descenttest.minimum_eigensolvers.test_adapt_vqe-> deprecation warning handling only