Closed maxwell04-wq closed 1 year ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.
View / edit / reply to this conversation on ReviewNB
ElePT commented on 2023-06-01T15:05:57Z ----------------------------------------------------------------
I think that the title "Qiskit Gradient Framework using Primitives" is a bit more fitting, "The Gradient Framework" sounds too generic for me.
Also, I like how concise the tutorial goal is, but I think it could even be made into a single sentence, and include a link to the API ref. for the primitives:
This tutorial demonstrates the use of the qiskit.algorithms.gradients
module to evaluate quantum gradients using the Qiskit Primitives
Finally, if it is not longer useful, could you please remove the comment? It is fine to have some comments while drafting the tutorial, but we should avoid them in the final version :)
maxwell04-wq commented on 2023-06-01T19:32:19Z ----------------------------------------------------------------
Incorporated.
View / edit / reply to this conversation on ReviewNB
ElePT commented on 2023-06-01T15:05:58Z ----------------------------------------------------------------
I would call this section something like "A quick refresher on Qiskit Primitives".
Qiskit Primitives is a new framework for executing quantum jobs. Instead of binding parameters to quantum circuits, theprimitives
module uses either aSampler
or anEstimator
class to evaluate the quantum circuits. These Primitive classes take the circuits, the observable hamiltonians, and the circuit parameters and return the results of quantum simulations.
And I would suggest a few changes to the sentence above, I know that explaining the primitives is still a bit tricky, but to make it better it is important to be precise describing what the primitives do:
The Qiskit Primitives work as an abstraction level between algorithms and (real/simulated) quantum devices. Instead of having to manually deal with tasks such as parameter binding or circuit transpilation, theprimitives
module offers aSampler
and anEstimator
class that take the circuits, the observable Hamiltonians, and the circuit parameters and return the sampling distribution and the computed expectation values respectively.
Now, for the following section:
Qiskit Primitives provides two classess for evaluating the circuit:
The Estimator
class allows to evaluate the expectation values of the observables.
The Sampler
class allows to evaluate the quasi-probability distribution of the observables.
I would say:
qiskit.primitives
instead of "Qiskit Primitives"Estimator
class allows to evaluate expectation values of observables with respect to states prepared by quantum circuitsSampler
class returns quasi-probability distributions as a result of sampling quantum circuits
maxwell04-wq commented on 2023-06-01T19:37:37Z ----------------------------------------------------------------
Incorporated.
View / edit / reply to this conversation on ReviewNB
ElePT commented on 2023-06-01T15:05:59Z ----------------------------------------------------------------
Overall, I really like this section, but to push it to the next level I would restructure it a little bit, and explain the base classes first, include classical gradients and talk about the QGT. This would look like this:
Thealgorithms.gradients
module contains both circuit gradients and circuit metrics. The gradients extend theBaseEstimatorGradient
andBaseSamplerGradient
base classes. These are abstract classes on top of which different gradient methods have been implemented. The methods currently available in this module are:
Additionally, the module offers reverse gradients for efficient classical computations.
The metrics available are based on the notion of the Quantum Geometric Tensor (QGT). There is aBaseQGT
class (Estimator
-based) on top of which different QGT methods have been implemented:
As well as a Quantum Fisher Information class (QFI) that is initialized with a reference QGT implementation from the above list.
Incorporated.
View / edit / reply to this conversation on ReviewNB
ElePT commented on 2023-06-01T15:06:00Z ----------------------------------------------------------------
I love that you have made a diagram, this shows a lot of effort :) I think we can replace Qiskit Algorithms with Qiskit Primitives at the bottom (as the gradients are built on top of that), and add the Reverse Gradient to the list (I know that I mentioned that a reverse gradient tutorial was optional, we don't have to go into detail, but I think that we should at least name them and show where they fit in the overall structure).
I am not so sure about the "Circuit" in "Circuit Gradients" and "Circuit Metrics", we can maybe drop it.
maxwell04-wq commented on 2023-06-01T20:12:42Z ----------------------------------------------------------------
Incorporated.
View / edit / reply to this conversation on ReviewNB
ElePT commented on 2023-06-01T15:06:01Z ----------------------------------------------------------------
We are trying to move away from having import blocks at the beginning of the tutorial, instead, we encourage to add the relevant imports at the top of the cell where they are used. You can see this, for example, in this tutorial https://qiskit.org/ecosystem/ibm-runtime/tutorials/grover_with_sampler.html
maxwell04-wq commented on 2023-06-01T20:28:57Z ----------------------------------------------------------------
Incorporated.
View / edit / reply to this conversation on ReviewNB
ElePT commented on 2023-06-01T15:06:02Z ----------------------------------------------------------------
Given that we no longer talk about the difference between First and Second Order gradients, maybe just "Gradients" fits this section better.
Here, and in the following subsections, I think it's important to always be very clear about when we are showing estimator gradients and when we are using sampler gradients. You already do this in the Parameter Shift Gradients section, but in the other sections you don't mention that you are talking about estimator gradients.
maxwell04-wq commented on 2023-06-01T20:30:46Z ----------------------------------------------------------------
I'm adding a note in the Sampler example that all the following examples will be with estimators.
View / edit / reply to this conversation on ReviewNB
ElePT commented on 2023-06-01T15:06:03Z ----------------------------------------------------------------
Citing the spsa paperwould be nice.
maxwell04-wq commented on 2023-06-01T20:35:20Z ----------------------------------------------------------------
Done.
View / edit / reply to this conversation on ReviewNB
ElePT commented on 2023-06-01T15:06:04Z ----------------------------------------------------------------
similar comment to the one I left above, we should spread out the imports.
maxwell04-wq commented on 2023-06-01T20:37:15Z ----------------------------------------------------------------
Done.
View / edit / reply to this conversation on ReviewNB
ElePT commented on 2023-06-01T15:06:06Z ----------------------------------------------------------------
Line #1. #Make circuit copies for different VQEs
View / edit / reply to this conversation on ReviewNB
ElePT commented on 2023-06-01T15:06:07Z ----------------------------------------------------------------
This problem can also be tackled with the Sampler
-based SamplingVQE
, as this algorithm requires the Hamiltonian to be diagonal (this is not the case for regular VQE
). In this case, the algorithm will use a Sampler
-based gradient.
View / edit / reply to this conversation on ReviewNB
ElePT commented on 2023-06-01T15:06:08Z ----------------------------------------------------------------
Line #3. vqe = SamplingVQE(sampler=sampler, ansatz=wavefunction_1, optimizer=optimizer)
I think that the point of this example was to show the use of a sampler gradient, right? but no custom gradient is set here
SamplingVQE
does not take a gradient as a parameter. Dropping this example.
I'm adding a note in the Sampler example that all the following examples will be with estimators.
View entire conversation on ReviewNB
SamplingVQE
does not take a gradient as a parameter. Dropping this example.
View entire conversation on ReviewNB
@ElePT I have incorporated the suggested changes. Please review it at your earliest convenience.
Thanks Fatima, my response is going to be a bit slower this week, but I will get around to reviewing before the Unitary Hack deadline :)
@ElePT as UnitaryHack ends in about half a day, can you please review the PR so that I can claim the bounty?
@maxwell04-wq open PRs reviewed also later in the week will be considered for unitaryHACK.
Summary
Fixes #1390
Details and comments
opflow.gradients
toalgorithms.gradients
BaseEstimator
andBaseSampler
classes from Primitives for gradient evaluation