Add SamplerQNN class using terra primitives
#436
Conversation
…ine-learning into primitives-qnn Merge latest changes from main
|
Why would this not be in |
Yes, I totally agree, I just moved my |
…ine-learning Merge latest changes in main
| self, | ||
| sampler: BaseSampler, | ||
| circuit: QuantumCircuit, | ||
| input_params: List[Parameter] | None = None, |
There was a problem hiding this comment.
Do we need List (list) or could we go with Sequence as bit more generic.
There was a problem hiding this comment.
I have changed to Sequence, but I am wondering about the discussion about the Sequence type not including np.ndarray from here: Qiskit/qiskit#8812 (comment)
There was a problem hiding this comment.
Even though it says Sequence the primitives will keep supporting np.ndarray. Some people suggest moving back to Iterable, however these can actually be infinite (e.g. itertools.count), which raises concerns of secutiry, memory usage, and potential DOS attacks among others.
An alternative that I have proposed in the past is to move to Collection which does include np.ndarray but without being infinite. I personally like this approach, even though Mappings are included so a dictionary could be passed.
For now, we are sticking to Sequence in the primitives since it is the least general. That way, generalizing it later on in the future will not be a breaking a change.
| input_params: The parameters of the circuit corresponding to the input. | ||
| weight_params: The parameters of the circuit corresponding to the trainable weights. |
There was a problem hiding this comment.
I think the descriptions here could include what happens for None (default) case.
| # set sampler --> make property? | ||
| self.sampler = sampler |
There was a problem hiding this comment.
Public instance vars are what we have been moving towards - they do need an Attributes: docstring part in the class description though to make them visible in the API ref,
There was a problem hiding this comment.
I wonder if I should migrate all the existing properties to public instances then, I guess this would also affect the NeuralNetwork base class, because some properties like input_gradients are inherited from it.
| where an interpret function is provided. See constructor | ||
| for more details. | ||
| """ |
There was a problem hiding this comment.
See constructor for more details.
I think there is less stated there than here from what I can see. Rather than "only used" would it be better to revert it say "its ignored if no custom interpret method is provided where the shape is taken to be 2^circuit.num_qubits". If we state its ignored I guess the logger.warning is still valid - ideally this would not be set alone.
There was a problem hiding this comment.
Updated in the constructor and here in the property as you suggested.
|
|
||
|
|
||
| class SamplerQNN(NeuralNetwork): | ||
| """A Neural Network implementation based on the Sampler primitive. |
There was a problem hiding this comment.
I'll comment here even though I think where it could be altered is the EstimatorQNN line. If I look at the summary
then EstimatorQNN is the only one in the list with neural network not with caps. Its pretty minor in the scheme of things just something that stood out with them next to one another like that,
There was a problem hiding this comment.
Updated to make lower case in the QNNs. I don't know why caps.
| class SamplerQNN(NeuralNetwork): | ||
| """A Neural Network implementation based on the Sampler primitive. | ||
| The ``Sampler QNN`` is a neural network that takes in a parametrized quantum circuit |
There was a problem hiding this comment.
This comes out with a space like the source above - was that intended?
As a highlight term I would have thought not.
| :class:`~qiskit.algorithms.gradients.ParamShiftSamplerGradient` will be used. | ||
| input_gradients: Determines whether to compute gradients with respect to input data. | ||
| Note that this parameter is ``False`` by default, and must be explicitly set to | ||
| ``True`` for a proper gradient computation when using ``TorchConnector``. |
There was a problem hiding this comment.
In EstimatorQNN we made TorchConnector a class ref so it was a link not just highlighted
There was a problem hiding this comment.
Updated the link to TorchConnector
| ) | ||
|
|
||
| @property | ||
| def circuit(self) -> QuantumCircuit: |
There was a problem hiding this comment.
Even though there are just getters here (circuit, input & weight params) what are being returned are the internal references to these mutable objects. Hence while you cannot change the whole item the item itself can be mutated by what is given back. While the internal reference could be copied maybe its not needed - its not done across the constructor either unless the list() cast needs to do more.
There was a problem hiding this comment.
I'd avoid copying objects until it is required. There are many other ways how to shoot in the foot.
| ``True`` for a proper gradient computation when using ``TorchConnector``. | ||
| Raises: | ||
| QiskitMachineLearningError: Invalid parameter values. | ||
| """ |
There was a problem hiding this comment.
I will note that many of the types in the arg list are not clickable links here - just the purple colored ones. I am not sure whether its an aspect of the new syntax or not. If I look at CircuitQNN QuantumCircuit is link there whereas not here. I'll check with something newer from Nature, though one from 10 days ago has a similar issue in terms of some things are links others not.
There was a problem hiding this comment.
I don't know why as well. Perhaps due to the new syntax.
| (see :class:`~qiskit.algorithms.gradients.BaseSamplerGradient`) to enable runtime access and | ||
| more efficient computation of forward and backward passes more efficiently. | ||
| The new Sampler QNN exposes a similar interface to the Circuit QNN, with a few differences. |
There was a problem hiding this comment.
Is there a reason not to have SamplerQNN and CircuitQNN instead of having spaces in there. The former will match a search the other not so much. I see it was done as Oflow QNN with a space in the Estimator release note though EstimatorQNN I see just as the one word.
There was a problem hiding this comment.
I don't see a reason. Updated with the links.
I can separately update the EstimatorQNN reno if required. Let me know.
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
* Add primitives branch to CI workflow * added pocs * init fixed * more init * Add qnns PoC * Establish qnn branch * Add fwd unit test * Refactor sampler qnn * Undo kernel changes * Start adding new type hint style * Move to neural_networks * Runnable tests * Update typehints, refactor * Add new unittests, fix code * Add gradient unit test * Add torch tests * Remove unwanted change circuitQNN * Remove utils * Fix style, lint * Add VQC * Fix mypy * fix mypy * fix mypy * fix mypy * Restore workflow * Restore workflow * Update .github/workflows/main.yml * Fix mypy * Fix CI (hopefully) * Update requirements * Add sparse support * Fix style etc * Add type ignore * Fix style? * skip test if not sparse * Add type ignore * Fix mypy * Make keyword args * Make keyword args * Apply review comments * Fix tests, final refactor, add docstring * Add Sampler QNN * Update qiskit_machine_learning/algorithms/classifiers/vqc.py Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com> * Apply reviews vqc * Apply reviews * Fix neko test * Fix spell check * Update qiskit_machine_learning/neural_networks/sampler_qnn.py * Add try-catch * Add deprecations * Update tests * Fix tests * Filter warnings * Fix filter * fix black, pylint * update docstring * pulled the method up, modern type hints * fix spell * Update qiskit_machine_learning/neural_networks/sampler_qnn.py Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com> * Update qiskit_machine_learning/neural_networks/sampler_qnn.py Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com> * code review Co-authored-by: Manoel Marques <manoel.marques@ibm.com> Co-authored-by: Gian Gentinetta <gian.gentinetta@gmx.ch> Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com> Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com> Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>
7 participants
Summary
This PR presents a reference implementation of a Qiskit
NeuralNetworkthat leverages the newly introduced primitives (so far, this class is based on the terra interface). Closes #400.Details and comments
This PR is blocked by the release of Qiskit-terra that will include gradients and changes in the primitives interface.
Open Points of Discussion