-
Notifications
You must be signed in to change notification settings - Fork 575
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
Add template to prepare quantum states of molecules using the excitation operations #1383
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1383 +/- ##
=======================================
Coverage 98.19% 98.20%
=======================================
Files 157 158 +1
Lines 11729 11780 +51
=======================================
+ Hits 11517 11568 +51
Misses 212 212
Continue to review full report at Codecov.
|
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.
Docstrings need changes and so do the arguments of the template, which I think should be just number of qubits and number of particles.
I'll look at tests once these changes are made, since they will affect testing
.github/CHANGELOG.md
Outdated
The new template reduces significantly the number of operations | ||
and the depth of the quantum circuit with respect to the traditional UCCSD | ||
unitary. The performance of quantum chemistry simulations should benefit | ||
from this new implementation. |
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.
You can remove the last sentence, which is redundant: it's of course beneficial to significantly reduce resource requirements.
It would be good to add a short snippet of example usage, as we do for other contributions
|
||
|
||
class AllSinglesDoubles(Operation): | ||
r"""Apply the particle-conserving :class:`~.pennylane.SingleExcitation` and |
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 not clear enough in saying that the template builds a circuit consisting of ALL such excitation gates, which conserve spin
:class:`~.pennylane.DoubleExcitation` operations, implemented as Givens rotations, | ||
to prepare quantum states of molecules beyond the Hartree-Fock approximation. | ||
|
||
In the Jordan-Wigner representation the qubit state :math:`\vert 0 \rangle` or |
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 think this paragraph is trying to explain too much and it's probably better to remove.
We can direct the user to the documentation for the excitation gates, we don't need to re-explain everything here
Co-authored-by: ixfoduap <40441298+ixfoduap@users.noreply.github.com>
Co-authored-by: ixfoduap <40441298+ixfoduap@users.noreply.github.com>
…AI/pennylane into template-excitation-operations
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.
Looks good to me! 🚀
Only main suggestion is whether we want to add a shape
method, but this may not be necessary since the users get a clear error when passing the wrong shape of weights
.github/CHANGELOG.md
Outdated
|
||
@qml.qnode(dev) | ||
def circuit(weights, hf_state, singles, doubles): | ||
qml.templates.AllSinglesDoubles(weights, wires, hf_state, singles, doubles) |
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 really long! I guess there's not much we can do about it, but it's a shame
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.
Could import the template and save the qml.templates
part? I think we often do that in docstrings
# we can extract the numpy representation here | ||
# since hf_state can never be differentiable |
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 comment doesn't fit in one line?
|
||
# we can extract the numpy representation here | ||
# since hf_state can never be differentiable | ||
self.hf_state = qml.math.toarray(hf_state) |
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.
Nice! I need to get used to using qml.math
) | ||
) | ||
|
||
shape = qml.math.shape(weights) |
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 optional, but we could consider adding a shape()
method that the user can employ to obtain the shape of the input parameters, as is done here
https://pennylane.readthedocs.io/en/latest/_modules/pennylane/templates/layers/cv_neural_net.html#CVNeuralNetLayers.shape
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.
Actually, I am strongly in favour of shape()
being added here. It is frustrating from a user point of view for some templates to have this method, and some to not :)
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.
If I understand correctly, do we want to add a shape()
method that receives the lists singles
and doubles
and returns ``shape = (len(singles) + len(doubles)) ?
@mariaschuld your instructions here would be valuable :-).
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, the shape method would have the @staticmethod
decorator and does not take self
, which means that all information it needs have to be derived from the argument that shape()
gets (since you cannot do self.xyz
).
If the shape can be derived from anything other than singles
and doubles
, like the number of qubits or something the user should know about the circuit, I would prefer this - especially if you make the singles
and doubles
optional as discussed above!
"'singles' and 'doubles' lists can not be both empty; got singles = {}, doubles = {}".format( | ||
singles, doubles | ||
) | ||
) |
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's still a matter of taste, but we are trying to keep the checks to an absolute minimum, because they are used in every qnode call...
My thinking is that we should check tensor dimensions (since this is an annoying error to see), and things that are theoretically tricky. So if this check is something a user with a look at the docs can get right, maybe we should delete it?
Non-blocking comment though
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.
Good point. My concern in removing this check is that the user may want to call this template without passing singles
and doubles
. In this case, the template will not raise an error, will only prepare the HF state and numbers will come out (even though the state the user is preparing is trivial). I vote to keep it :-).
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 didn't know that not a
evaluates to True if a=None
, interesting!
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.
A very nice PR. I am only requesting changes because I'd like to
- have the
shape
method added (plus a test) - if it makes sense, make
singles
anddoubles
an optional argument as proposed by JM
If you have concerns about any of these, I am happy to be convinced otherwise :)
Co-authored-by: Maria Schuld <mariaschuld@gmail.com>
…AI/pennylane into template-excitation-operations
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, one last round: I think there is a twister in the shape
function, and you forgot to test it (I think) :)
Else I agree with all your arguments!
Co-authored-by: Maria Schuld <mariaschuld@gmail.com>
Co-authored-by: Maria Schuld <mariaschuld@gmail.com>
This reverts commit 7798409.
Context:
This PR adds the new template
AllSinglesDoubles
which uses theSingleExcitation
andDoubleExcitation
operations recently implemented in the PRs 1121 and 1123 to prepare correlated states of molecules.Description of the Change:
Add the AllSinglesDoubles class in the file
~/templates/subroutines/all_singles_doubles.py
and the corresponding unit tests in~/tests/templates/test_subroutines/test_all_singles_doubles.py
Benefits:
The new template reduces significantly the number of operations and the depth of the quantum circuit with respect to the traditional UCCSD unitary.