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

Added function to remove qubits using symmetries #356

Merged
merged 29 commits into from Jun 6, 2018

Conversation

Projects
None yet
4 participants
@sammcardle30
Contributor

sammcardle30 commented Jun 1, 2018

Hi OpenFermion developers,

I’ve made a function to reduce 2 qubits from Bravyi-Kitaev mapped Hamiltonians using symmetries, as described in arXiv:1701.08213. It’s been useful for my research so far, so I thought that it would be a good file to contribute.
I’ve included unit tests of the function, and have tried to adhere to the style guide- but please do let me know if there’s anything which needs changing.
I’d also be happy to add a tutorial, perhaps of using this function and NMO’s to reduce the LiH Hamiltonian from 12 to 6 qubits, as described in arXiv:1803.10238.

Best regards,

Sam McArdle

Added function to remove qubits using symmetries
Added function to remove 2 qubits from Bravyi-Kitaev mapped Hamiltonians using the method described in arXiv:1701.08213. Also added unit tests for the function.

@googlebot googlebot added the cla: yes label Jun 1, 2018

@googlebot

This comment was marked as outdated.

googlebot commented Jun 1, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Jun 1, 2018

@babbush babbush added cla: yes and removed cla: no labels Jun 1, 2018

@googlebot

This comment has been minimized.

googlebot commented Jun 1, 2018

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@googlebot

This comment was marked as outdated.

googlebot commented Jun 1, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Jun 1, 2018

@babbush babbush added cla: yes and removed cla: no labels Jun 1, 2018

@googlebot

This comment has been minimized.

googlebot commented Jun 1, 2018

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@babbush

This looks like a nice pull request, thanks! There are a number of minor style guide comments. I'd recommend running the pep8 linter which should pick most of them up (I am sure I missed some). There are also a couple design pattern solutions. If you address these comments I think we should be good to merge after one more light round of review.

You can also feel free to edit the README and NOTICE files to include your name in the authors list in the alphabetical section.

using conservation of electron number and conservation
of electron spin. As described in arXiv:1701.08213.
"""

This comment has been minimized.

@babbush

babbush Jun 1, 2018

Contributor

there should be one space here not two.

from openfermion.utils import reorder, up_then_down, prune_unused_indices
import numpy

This comment has been minimized.

@babbush

babbush Jun 1, 2018

Contributor

only two spaces here, not six.

def remove_symmetry_qubits(fermion_hamil, active_orbitals, active_electrons):

This comment has been minimized.

@babbush

babbush Jun 1, 2018

Contributor

The name of this function suggests that it takes a QubitOperator and removes qubits. Since it actually takes a FermionOperator I suggest naming the function something like "symmetry_conserving_bravyi_kitaev" or something of that sort.

def remove_symmetry_qubits(fermion_hamil, active_orbitals, active_electrons):
""" Returns the qubit Hamiltonian for the molecular fermionic

This comment has been minimized.

@babbush

babbush Jun 1, 2018

Contributor

nothing about this strategy requires the Hamiltonian is for a molecule. For instance, this would work perfectly well for the Hubbard model.

of electron spin and number, as described in arXiv:1701.08213.
Args:
fermion_hamil: A fermionic molecular hamiltonian obtained

This comment has been minimized.

@babbush

babbush Jun 1, 2018

Contributor

it would be preferred if you just wrote out "fermion_hamiltonian".

import unittest
import openfermion
import openfermionpyscf

This comment has been minimized.

@babbush

babbush Jun 1, 2018

Contributor

these are awesome tests but unfortunately you cannot import openfermionpyscf inside of openfermion (but you can import openfermion inside of openfermionpyscf). There are many reasons for this. Among them: some people want to use openfermion without installing openfermionpyscf / pyscf (for instance, if you use windows where it is impossible to install pyscf). And we can't have tests failing for those users. So if you want to have tests that depend on openfermionpyscf, you'd need to put them in the openfermionpyscf repo, which you are welcome to do. Otherwise, perhaps you should hardcode the outputs so that these tests can run without openfermionpyscf. Thanks!

import openfermionpyscf
from openfermion.hamiltonians import MolecularData
from openfermionpyscf import run_pyscf
from openfermion.utils import eigenspectrum

This comment has been minimized.

@babbush

babbush Jun 1, 2018

Contributor

alphabetical imports: ops comes before utils.

from openfermion.ops import FermionOperator, QubitOperator
from openfermion.transforms import get_fermion_operator
import _remove_symmetry_qubits

This comment has been minimized.

@babbush

babbush Jun 1, 2018

Contributor

you should edit the init file in utils to import your function and then import it through utils.

import _remove_symmetry_qubits

This comment has been minimized.

@babbush

babbush Jun 1, 2018

Contributor

delete extra space.

lih_sto_numorb-2)
if __name__ == '__main__':

This comment has been minimized.

@babbush

babbush Jun 1, 2018

Contributor

we do not have main in any of our files. The tests are run through the pytest module.

@babbush

This comment has been minimized.

Contributor

babbush commented Jun 1, 2018

note that tests fail since we do not build openfermionpyscf during the continuous integration testing of openfermion, for reasons I explain in the review.

@sammcardle30

This comment has been minimized.

Contributor

sammcardle30 commented Jun 1, 2018

Hi Ryan,
Brilliant, thank you for all of your feedback and suggestions! What is the best way for me to make the changes that you've suggested and resubmit the files for another review?
The reason I've reordered the orbitals and used the BK tree mapping rather than the default ordering and mapping is because only the former seemed to leave Hamiltonians where the middle and final qubits were acted on by the Z or I operators; using the latter combination didn't necessarily give 2 qubits which I could reduce. I can have a think about how the algorithm would change if the qubits are instead ordered up-down-up-down, if you'd prefer? I'll also try to fix the test function so that it doesn't include pyscf. Can I use the approach in this test https://github.com/quantumlib/OpenFermion/blob/master/src/openfermion/transforms/_binary_codes_test.py
where the LiH Hamiltonian is just loaded in?
Thanks again,
Sam

@babbush

This comment has been minimized.

Contributor

babbush commented Jun 1, 2018

Hi Sam,

To make changes, just push additional commits to the git branch from which you submitted this pull request. It will automatically push the changes here. It sounds like you prefer the up-then-down ordering because it is convienient to perform the symmetry reduction. In that case, just transform the Hamiltonian to up-then-down, apply the symmetry reduction, then transform back to even-odd. Perhaps I misunderstand something though and the issue is trickier than that. Yes, it is fine to use the approach where the LiH Hamiltonian is loaded in. We keep those preloaded files around precisely for testing purposes like this. Thanks!

@sammcardle30

This comment has been minimized.

Contributor

sammcardle30 commented Jun 1, 2018

Hi Ryan,
Perfect, thank you. Ah ok, that sounds like the best approach then; I'll stick with the all up- all down ordering to make the symmetry reduction easier, and then transform the qubit Hamiltonian back into the default orbital ordering, as you suggest. That's ideal then, I'll change the tests to use the presaved files.
I'll make all of the changes over the next few days, and resubmit them on Monday.
Thanks,
Sam

@sammcardle30

This comment has been minimized.

Contributor

sammcardle30 commented Jun 4, 2018

Hi Ryan,

I'm just working on integrating the changes you've suggested, and have run into a complication with the ordering of spins.
The method I've implemented is that described in 'Tapering off qubits to simulate fermionic Hamiltonians' by Bravyi et al. It relies on ordering the orbitals all spin up, then all spin down. This is because the middle and final rows of the BK transform matrix have 1's as half or all of their entries, respectively. Therefore the middle and final entries of the BK transformed state vector are the number of spin up electrons (mod 2) and the total number of electrons (mod 2), respectively. Because these quantities are conserved, the Hamiltonian won't change them, so only acts on the middle and final qubits with I or Z operators. The effect of these operators can be precomputed, leaving those 2 qubits redundant.
I've looked at your suggestion of using the "reorder" function to swap the ordering back to up-down. However, because the qubit Hamiltonian obtained is in the BK mapping, and so each qubit may store a combination of orbitals, it's in general non-trivial to just swap the indices on the qubit operators to reorder the spins. From what I can see, the best way to get back into the up-down ordering would be to apply the inverse BK transform to the reduced Hamiltonian, reorder the fermion operator, and then BK transform again. However, as OpenFermion doesn't currently have a reverse BK transform function, I haven't been able to check that this indeed works.

I was wondering if you had any suggestions about the best way to proceed? Would you prefer a workaround to restore the up-down ordering, or is it sufficient to make it clear in the function description (and/or name) that it reorders the orbitals?

Thanks for your help,

Sam

@googlebot

This comment has been minimized.

googlebot commented Jun 4, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Jun 4, 2018

sammcardle30 added some commits Jun 4, 2018

Made required changes
Included style changes, removed dependence on pyscf
@babbush

This comment has been minimized.

Contributor

babbush commented Jun 4, 2018

Hi Sam,

Shouldn't it be possible to just identify which qubits are acted upon trivially if BK is performed in the even-odd ordering? If not, let me ask this: is it even the case that each qubit encodes orbitals of a well defined spin post-symmetry reduction? If so, do we still have the same number of up spins as down spins?

@sammcardle30

This comment has been minimized.

Contributor

sammcardle30 commented Jun 5, 2018

Hi Ryan,
When working in the default even-odd ordering, only the final qubit is guaranteed to be acted upon trivially, as it stores the sum of the occupation numbers of all of the orbitals (mod2). However, no other qubit will necessarily be acted upon trivially. So we can in general only reduce one qubit, rather than two, if we don't initially reorder the orbitals (verified with H2 in ccpvdz basis). In some circumstances we are able to reduce two qubits without reordering the orbitals (this is true for LiH and H2 in the STO bases), but in general we'll need to reorder the orbitals to remove two qubits.
After symmetry reduction (in the all up- all down ordering) the qubits still encode orbitals of a well defined spin and we still have an equal number of up and down spins. The symmetry reduction makes use of the fact that in this ordering, the occupancy of the highest up and down orbitals (the final two orbitals in the default ordering) are stored only in the middle and final qubits. The middle qubit stores the sum of the occupancies of the up orbitals (mod2). The final qubit stores the sum of the occupancies of all of the orbitals (mod2). As the value of these sums stays fixed throughout the simulation, we eliminate these qubits, so I suppose in a loose sense we have dropped the highest up and down orbitals from the simulation, and are inferring their occupation by enforcing conservation of spin and electron number.
Best regards,
Sam

sammcardle30 added some commits Jun 5, 2018

@sammcardle30

This comment has been minimized.

Contributor

sammcardle30 commented Jun 5, 2018

From the checks that I've done, using bravyi_kitaev_tree seems to generalise. That's certainly something I'd be happy to look into, although the fact that there aren't 2 qubits acted upon trivially for LiH under the ordinary BK transform means the method would seemingly have to change from that of the IBM Tapering paper.

Brilliant, thank you for all of your help- I've moved the files to transforms now, as you suggest.

@kevinsung

This comment has been minimized.

Collaborator

kevinsung commented Jun 5, 2018

You're saying that Table I of the "Tapering" paper uses bravyi_kitaev_tree instead of bravyi_kitaev?

@babbush

This comment has been minimized.

Contributor

babbush commented Jun 5, 2018

As a minor historical note - this idea to remove the two qubits acted upon trivially, is actually not due to the Bravyi paper. Bravyi's paper explains why there are two qubits acted upon trivially, but the original observation was made by us in Phys. Rev. X 6, 031007, which was on arXiv well over a year before Bravyi's paper. There, in Appendix A, we discuss this observation. Note that I used the standard BK transform, not the tree transformed, and it seemed to work at least for H2. It is rather mysterious to me why you'd need the tree transform for this to work for LiH but if you have examined each qubit in that Hamiltonian closely then I'll take your word for it.

@sammcardle30

This comment has been minimized.

Contributor

sammcardle30 commented Jun 5, 2018

I'm not sure about Table 1- the part of the paper I've been working from is section 7. They reference the original Bravyi and Kitaev paper, which I believe corresponds to the usual bravyi_kitaev function in OpenFermion (although they refer to it as the binary tree mapping, so perhaps they are using the BK_tree version) ? I would have expected the ordinary bravyi_kitaev function to work (it does for H2 in the sto basis, as Ryan has noted above), but for whatever reason it doesn't seem to with LiH (or ccpvdz H2, and presumably other molecules).

Ah, apologies Ryan- I'll make sure to reference both papers next time I change the file.

@kevinsung

This comment has been minimized.

Collaborator

kevinsung commented Jun 5, 2018

They explicitly define their mapping in Eq. (50), and this corresponds to bravyi_kitaev, not bravyi_kitaev_tree.

sammcardle30 added some commits Jun 5, 2018

@sammcardle30

This comment has been minimized.

Contributor

sammcardle30 commented Jun 5, 2018

In that case, I'm not sure why using bravyi_kitaev() on the fermionic LiH Hamiltonian doesn't produce 2 qubits that are acted upon trivially - I agree that we would expect it to.

sammcardle30 added some commits Jun 5, 2018

@babbush

This comment has been minimized.

Contributor

babbush commented Jun 5, 2018

Hi Sam,

We are very close to being to merge but it seems that your test coverage is not high enough. Only 75% of the lines of code in your main file are covered by tests. You'll need to add a few more to bring the test coverage up high enough so that we can merge. You can use pytest to determine which lines are currently covered and which ones aren't.

@sammcardle30

This comment has been minimized.

Contributor

sammcardle30 commented Jun 5, 2018

Hi Ryan,
I'm just adding some more tests now, which will hopefully cover the majority of the remaining code.

sammcardle30 added some commits Jun 5, 2018

Added tests to increase coverage
Added test of LiH in a reduced active space.
@sammcardle30

This comment has been minimized.

Contributor

sammcardle30 commented Jun 6, 2018

Hi Ryan,
Coverage is now at ~92%, which is as high as I can get it without access to additional saved molecules- is that sufficiently high?

@babbush

This comment has been minimized.

Contributor

babbush commented Jun 6, 2018

Hi Sam,

Normally we don't like to merge PRs that lower test coverage by more than 0.01% and this one decreases overall coverage by 0.05%. However, this PR has gone on for a while so perhaps we can make an exception. But let me ask this: are you aware of which lines are not covered? If so, what functionality is not covered and why would you need additional molecule files to test them?

@babbush babbush added cla: yes and removed cla: no labels Jun 6, 2018

@googlebot

This comment has been minimized.

googlebot commented Jun 6, 2018

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@sammcardle30

This comment has been minimized.

Contributor

sammcardle30 commented Jun 6, 2018

The two uncovered lines are if clauses which are entered when (the number of active electrons % 4) is equal to 1 or 3. I suppose I could enter into these clauses by changing the active space of LiH such that it has 1 or 3 active electrons, but as this would result in an imbalanced Hamiltonian, it wouldn't make sense to apply the spin reduction, so the test would be somewhat for the sake of just increasing the coverage.

@babbush babbush merged commit 68f3638 into quantumlib:master Jun 6, 2018

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.05%) to 99.651%
Details
cla/google CLAs have been manually verified by Googler who has set the 'cla: yes' label
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@babbush

This comment has been minimized.

Contributor

babbush commented Jun 6, 2018

Okay, I merged the PR. If you want to be added to the release paper please email me so I can send you the link.

@sammcardle30

This comment has been minimized.

Contributor

sammcardle30 commented Jun 6, 2018

Fantastic, thank you for helping me to get this function added Ryan!

gaming-hacker added a commit to gaming-hacker/OpenFermion that referenced this pull request Jul 18, 2018

Merge branch 'master' into v1
* master:
  Updated version and citation (quantumlib#375)
  Add period cutoff to Jellium plane wave hamiltonian.  (quantumlib#358)
  Added SFOpenBoson to the list of OpenFermion plugins (quantumlib#374)
  Function to normal order in chemistry convention (quantumlib#372)
  minor changes and pep8 fixes to ops (quantumlib#371)
  updated h5py requirements (quantumlib#370)
  Sparse Davidson (quantumlib#369)
  Grid indices fixes and tests (quantumlib#367)
  Fix bug of optional args for get_lowest_n() in Davidson algo (quantumlib#364)
  Rewrite get ground states by particle number (quantumlib#359)
  add ignore terms flag (quantumlib#368)
  Increased test coverage for remove_symmetry_qubits (quantumlib#365)
  Added function to remove qubits using symmetries (quantumlib#356)
  Linear operator (quantumlib#357)
  Add support for real eigenvectors when specified, up to the user to decide when it should work. (quantumlib#354)
  Added support for BosonOperator and BoseHubbard models (quantumlib#352)

# Conflicts:
#	examples/performance_benchmarks.py
#	src/openfermion/tests/_example_test.py
#	src/openfermion/utils/__init__.py
#	src/openfermion/utils/_davidson.py
#	src/openfermion/utils/_davidson_test.py
#	src/openfermion/utils/_sparse_tools.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment