-
Notifications
You must be signed in to change notification settings - Fork 603
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 function to taper Hartree Fock state for qubit tapering #2042
Conversation
Hello. You may have forgotten to update the changelog!
|
…o opt_symmetry_sector
…pennylane into taper_hartree_fock
Codecov Report
@@ Coverage Diff @@
## master #2042 +/- ##
==========================================
+ Coverage 99.14% 99.19% +0.04%
==========================================
Files 224 229 +5
Lines 17158 17586 +428
==========================================
+ Hits 17012 17445 +433
+ Misses 146 141 -5
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.
All is good to me when the few comments are addressed.
@@ -445,7 +445,7 @@ def string_to_pauli_word(pauli_string, wire_map=None): | |||
# Special case: all-identity Pauli | |||
if pauli_string == "I" * len(wire_map): | |||
first_wire = list(wire_map.keys())[0] | |||
return Identity(wire_map[first_wire]) | |||
return Identity(first_wire) |
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.
Is this done to prevent having Identity for a wire that has been removed during tapering?
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.
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.
Note: This condition was actually being tested in the unit tests but was able to pass that test because of the following scenario:
>>> qml.Identity("a").compare(qml.Identity(1))
True
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 might create a bug report and fix it in a separate PR. Up to you.
), | ||
], | ||
) | ||
def test_hf_energy(symbols, geometry, charge): |
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 a strong tests but seems complicated tome; it basically tests many functions. Not sure if we should keep it or just use reference tapered states and mention that those reference states have been checked by comparing the HF energy obtained with <\psi_{HF}| H |\psi_{HF}> for the tapered and untapered Hamiltonian and states.
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.
All is good for me. Just two minor tings:
i) why the pennylane/grouping/utils.py is modified (to prevent having Identity for non-existing wires)?
ii) not sure if test_hf_energy should be replaced with a function that tests the tapered stated with a ref state that has been verified with <state_tap_ref|H_tap|state_tap_ref>. Leave it up to you and others' opinion.
…o taper_hartree_fock
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.
The code looks fine to me, but I find the docstring for this function very confusing.
We should either explain things better, or possibl give simply a short description of what teh function does without explaining how it does it, e.g.,
Computes the Hartree-Fock state for a tapered Hamiltonian.
I prefer to explain
pennylane/hf/tapering.py
Outdated
The Hartree-Fock (HF) state for a molecule is transformed to a qubit observable under Jordan-Wigner (JW) | ||
transform. To do this, each occupied orbital in the original HF state is assigned with a fermionic creation | ||
operator and each unoccupied orbital is replaced with an identity operator. The JW transform is then applied to | ||
map the resulting fermionic operator to a qubit basis. |
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 find this docstring very confusing. It should be updated before merging
What does it mean that a state is transformed to an observable?
Do you mean that we have fermionic creation operators acting on orbitals, and these define the HF state? Then we transform these operators under JW to obtain a qubit operator that acting on the all-zero state produces the HF state. Then transforming this operator under the Clifford symmetries of the Hamiltonian gives a new operator, that can be defined on a reduced subset of qubits and, acting on the all-zero state in this subset creates the new Hf state (maybe up to some phases)?
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.
Yep! I have tried to rewrite it once again borrowing a bit from the way you have put it here.
pennylane/hf/tapering.py
Outdated
|
||
>>> symbols = ['H', 'H'] | ||
>>> geometry = np.array([[0.0, 0.0, -0.69440367], [0.0, 0.0, 0.69440367]]) | ||
>>> mol = qml.hf.Molecule(symbols, geometry) |
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 example looks very verbose and the output is confusing.
Can we simplify it a bit and use an example where the HF state has more than one qubit?
For example, maybe we use from pennylane import hf
to have hf.
instead of qml.hf
, use shorter variable names, maybe define n = mol.n_electrons
. This applies also I suppose to the example code in the changelog
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 have tried to reduce it. Also, I have updated the example to HeH+
which has a two-qubit tapered HF state.
|
||
pauli_map = {"I": qml.Identity, "X": qml.PauliX, "Y": qml.PauliY, "Z": qml.PauliZ} | ||
|
||
# build the untapered Hartree Fock 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.
Hmmm, is this always correct? I guess so. Makes me wonder how qml.qchem.hf_state()
does it, since this seems very compact
# build the untapered Hartree Fock state | ||
hf = np.where(np.arange(num_wires) < num_electrons, 1, 0) | ||
|
||
# convert the HF state to a corresponding HF observable under the JW transform |
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'm not sure I understand what this observable is. Again, something to consider clarifying in the docstring
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.
Code wise everything looks good, just a few clarifications and polishing.
|
||
|
||
def _binary_matrix(terms, num_qubits): | ||
def _binary_matrix(terms, num_qubits, wire_map=None): |
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.
@obliviateandsurrender, It might be worth adding a test to cover this new functionality
if wire_map is None: | ||
all_wires = qml.wires.Wires.all_wires([term.wires for term in terms], sort=True) | ||
wire_map = {i: c for c, i in enumerate(all_wires)} |
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.
Clean 🥇
num_wires (int): number of wires in the system for generating the Hartree-Fock bitstring | ||
|
||
Returns: | ||
array(int): tapered Hartree-Fock 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.
I don't fully understand this, what is the state in this case? It can't be a state vector or an operator. Is this instead meant to be a list of wires on which the HF state is initialized? Or is it some compact representation of the state (kind of like what we do with Hamiltonians)?
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.
Since we are aiming to taper off the qubits required for the simulations, we also need to transform our HF state so that it remains compatible with the new transformed Hamiltonian.
For example, the tapered Hamiltonian of the H3+
molecule acts non-trivially only on a subset of 4 qubits out of 6. So, consequently, we would then need to transform our HF state (which is previously defined on 6 qubits) to a new HF state (which is still a classical basis state) defined on this said subset.
…ennylane into taper_hartree_fock
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 to merge, but please clarify one sentence in the docstring before doing so.
pennylane/hf/tapering.py
Outdated
using the Jordan-Wigner encoding. This operator is then transformed using the Clifford operators :math:`U` | ||
obtained from the :math:`\mathbb{Z}_2` symmetries of the molecular Hamiltonian resulting in a qubit operator | ||
that acts non-trivially only on a subset of qubits. A new, tapered HF state is built on this reduced subset | ||
of qubits by putting the qubits that are acted on off-diagonally by the operator in the :math:`|1\rangle` |
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.
What does "putting the qubits that are acted on off-diagonally by the operator in the |1> state" mean? I find this confusing
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.
@ixfoduap I meant that we place all the qubits which are acted on by a Pauli-X
or Pauli-Y
operators in the excited state while keeping the rest (the ones acted by Pauli-Z and Identity) in the ground state.
Co-authored-by: ixfoduap <40441298+ixfoduap@users.noreply.github.com>
Context:
This PR adds function to obtain the tapered Hartree Fock state corresponding to the tapered Hamiltonian.
Description of the Change:
taper_hartree_fock
function, which tapers HF state based on symmetries generated from the molecular hamiltonian._generate_qubit_operator
function, to return single creation/anhilation operator under JW transform.Benefits:
Possible Drawbacks:
Related GitHub Issues: