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

Function to group qubit operators into tensor product basis sets #494

Merged
merged 14 commits into from Dec 14, 2018

Conversation

Projects
None yet
4 participants
@oscarhiggott
Copy link
Contributor

oscarhiggott commented Dec 12, 2018

Hi OpenFermion developers,

I've added a function that groups the terms in a QubitOperator into tensor product basis (TPB) sets, such that all the terms in each TPB set can be measured simultaneously in VQE with the same post rotations. This was shown to improve efficiency for some Hamiltonians in https://arxiv.org/pdf/1704.05018v2.pdf (SI section V(A)), with the improvement depending on the covariance between terms in a set on the specific state prepared (also discussed in https://arxiv.org/pdf/1509.04279v1.pdf section IV B 2 for commuting groups in general). I hope you find it useful and might like to include it - let me know if there's anything I should change.

Thanks,

Oscar

@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 12, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no label Dec 12, 2018

@kevinsung
Copy link
Collaborator

kevinsung left a comment

Thanks for the contribution, it looks useful! The main issues are reworking the logic to make it more readable and also to add better tests.

Show resolved Hide resolved src/openfermion/utils/_operator_utils.py Outdated
Show resolved Hide resolved src/openfermion/utils/_operator_utils.py
Show resolved Hide resolved src/openfermion/utils/_operator_utils_test.py
@babbush

This comment has been minimized.

Copy link
Contributor

babbush commented Dec 12, 2018

@oscarhiggott thanks for your contribution! As explained by the bot above, please do sign the CLA and then reply on this thread once you've done that. You can let us know if you have any questions.

@oscarhiggott

This comment has been minimized.

Copy link
Contributor

oscarhiggott commented Dec 12, 2018

@babbush and @kevinsung thanks for your help, and I've commited the changes you suggested Kevin. I also signed the CLA. Let me know if there's anything else I should change?

@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 12, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Dec 12, 2018

Show resolved Hide resolved src/openfermion/utils/_operator_utils.py
bases = list(bases)
if seed:
random.seed(seed)
random.shuffle(bases)

This comment has been minimized.

@kevinsung

kevinsung Dec 13, 2018

Collaborator

I know we haven't been consistent with this in other parts of the code base, but it's better not to have code that changes the global random state. Instead, seed a numpy.RandomState and use that to randomize the basis.

molecular_hamiltonian = molecule.get_molecular_hamiltonian()
hamiltonian = get_fermion_operator(molecular_hamiltonian)
cls.qubit_hamiltonian_bk = bravyi_kitaev(hamiltonian)
cls.qubit_hamiltonian_jw = jordan_wigner(hamiltonian)

This comment has been minimized.

@kevinsung

kevinsung Dec 13, 2018

Collaborator

The new tests you added are good; let's remove the ones that use molecular data to make these tests more self-contained.

@oscarhiggott

This comment has been minimized.

Copy link
Contributor

oscarhiggott commented Dec 14, 2018

Hi @kevinsung thanks for the suggestions - I've made the changes in commit 1021918. I also removed the randomize=False option in commit 7d22693 - now that we're using numpy.RandomState, my reason for including that option is no longer justified (deterministic behaviour can be achieved by setting a seed without changing the global random state).

sub_operators[new_basis] = sub_operators.pop(basis)
sub_operators[new_basis] += QubitOperator(term, coefficient)
else:
sub_operators[basis] += QubitOperator(term, coefficient)

This comment has been minimized.

@kevinsung

kevinsung Dec 14, 2018

Collaborator

Get rid of the inner if-else clause by replacing the entire outer else clause with

sub_operator = sub_operators.pop(basis)
sub_operator += QubitOperator(term, coefficient)
additions = tuple(op for op in term if op not in basis)
basis = tuple(sorted(basis + additions, key=lambda factor: factor[0]))
sub_operators[basis] = sub_operator

This comment has been minimized.

@oscarhiggott

oscarhiggott Dec 14, 2018

Contributor

Sure, done

@kevinsung
Copy link
Collaborator

kevinsung left a comment

LGTM with just some nits about whitespace. Thanks for the contribution!

Raises:
TypeError: Operator of invalid type.
"""

This comment has been minimized.

@kevinsung

kevinsung Dec 14, 2018

Collaborator

Nit: Get rid of this empty line.

r.shuffle(bases)

basis = _find_compatible_basis(term, bases)

This comment has been minimized.

@kevinsung

kevinsung Dec 14, 2018

Collaborator

Nit: Get rid of the empty lines surrounding the assignment to basis

@oscarhiggott

This comment has been minimized.

Copy link
Contributor

oscarhiggott commented Dec 14, 2018

Cheers, have removed the whitespace, would you like me to add myself to the readme?

@kevinsung

This comment has been minimized.

Copy link
Collaborator

kevinsung commented Dec 14, 2018

Yes, feel free to add yourself to the README and NOTICE

@oscarhiggott

This comment has been minimized.

Copy link
Contributor

oscarhiggott commented Dec 14, 2018

Great, have done, thanks for your help!

@kevinsung kevinsung merged commit 9c2e029 into quantumlib:master Dec 14, 2018

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 99.595%
Details

@oscarhiggott oscarhiggott deleted the oscarhiggott:group-qubit-operators branch Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment