Skip to content
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

Tapering off qubits using stabilizer conditions. #516

Merged
merged 24 commits into from Jun 5, 2019

Conversation

Projects
None yet
4 participants
@xabomon
Copy link
Contributor

commented May 15, 2019

This module tapers off qubits given a set of stabilizers.
Additionally, it reduces the number of terms of a Hamiltonian (operator) using stabilizer conditions.

The module allows the user to:

  1. Select which qubit to taper off by a given stabilizer.
  2. Keep the length of the Pauli string after reducing terms the same as the starting one.
  3. Return which qubits have been tapered off.

The module comes with documentation and test module.

Xavier Bonet-Monroig and Mark Steudtner

xabomon and others added some commits May 13, 2019

Create function to taper off qubits and reduce terms in a Hamiltonian…
… using stabilizer conditions, add test module for these functions, update init file to import them
Merge pull request #1 from xabomon/feature/tapering
Taper off and reduce terms by stabilizers.
Create function to taper off qubits and reduce terms in a Hamiltonian…
… using stabilizer conditions, add test module for these functions, update init file to import them
@googlebot

This comment has been minimized.

Copy link

commented May 15, 2019

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

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label May 15, 2019

@xabomon

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

I have signed the CLA!

@googlebot

This comment has been minimized.

Copy link

commented May 17, 2019

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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@msteudtner

This comment has been minimized.

Copy link

commented May 17, 2019

I'm ok with my comments being contributed.

@babbush babbush requested a review from ncrubin May 19, 2019

super().__init__(message)


def _check_commuting_stabilizers(stabilizer_list, msg, thres=1e-6):

This comment has been minimized.

Copy link
@ncrubin

ncrubin May 22, 2019

Collaborator

use openfermion.config.EQ_TOLERANCE unless there is a separate reason this threshold should be different.



def taper_off_qubits(operator, stabilizers, manual_input=False,
fixed_positions=[], output_tapered_positions=False):

This comment has been minimized.

Copy link
@ncrubin

ncrubin May 22, 2019

Collaborator

Use a non-mutable default type like None instead of empty list.

maintain_length=False,
output_fixed_positions=False,
manual_input=False,
fixed_positions=[]):

This comment has been minimized.

Copy link
@ncrubin

ncrubin May 22, 2019

Collaborator

It is unlikely that this method will be referenced abstractly later but for safety you should remove the list optional type in exchange for something that won't be mutable.

raise StabilizerError(msg)


def _fix_single_term(term, position, fixed_op, other_op, stabilizer):

This comment has been minimized.

Copy link
@ncrubin

ncrubin May 22, 2019

Collaborator

These "internal" functions still need argument doc strings. Later somebody maintaining this will find it easier to read if inputs and outputs are specified. Ditto on all following internal methods.

This comment has been minimized.

Copy link
@ncrubin

ncrubin May 22, 2019

Collaborator

In fact, what is the motivation for these being "_" named? Presumably these helper functions are likely to be used in further analysis for this line of research.

@@ -43,3 +43,6 @@
weyl_polynomial_quantization)
from ._remove_symmetry_qubits import (symmetry_conserving_bravyi_kitaev,
edit_hamiltonian_for_spin)

from ._qubit_tapering_from_stabilizer import (reduce_number_of_terms,

This comment has been minimized.

Copy link
@ncrubin

ncrubin May 22, 2019

Collaborator

move import into alphabetical order.

This comment has been minimized.

Copy link
@xabomon

xabomon May 22, 2019

Author Contributor

Hi Nicholas,

Thanks for the review, I'm happy to see that most of the things are in agreement with the code standards.

The init is in fact wrong. We originally put these code in transformations, but at the end decided to move it to utils, hence the transforms init is wrong, and the one in utils must be updated.
This will be done for the merge.

Best,
Xavi

@ncrubin

This comment has been minimized.

Copy link
Collaborator

commented May 22, 2019

Hey @xabomon thanks for the PR. Looks great. Can you merge with master so we get a fresh CI build and we can check out why this is failing. I suspect something with the init registration.

xabomon and others added some commits May 22, 2019

Remove transformation init, update utils init, adding functions in al…
…phabetic order, update changes as requested in the revision
@ncrubin

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

@xabomon looks like on of your tests is still attempting to import reduce_number_of_terms from transforms and not utils.

@xabomon

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

Hi @ncrubin,

Sorry for this, I just fixed the import in the test module.

Best,
Xavi

@xabomon

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Hi @ncrubin ,

After yesterday's commit I saw in the CI that one of the test was failing in the python2.7 version because I was defining it as an empty list, after changing the mutable variables to None.

I updated the test module that includes the new possible case, and change the old one.

Edit: Again the CI fails on the same test. I will try to solve this to make it full compatible with python 2.7.

Edit2: It seems that CI is failing in the python 2.7 version. From what I get from the logs the error comes because the test module does not recognize StabilizerError (instead returns TypeError).
Is there a known work-around?

Best,
Xavi

@ncrubin

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

I believe the rules for implementing super() are different in python2.7. I can look this weekend to resolve this if you don't solve by then!

@xabomon

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Hi Nick,

Don’t worry about it! I can’t look at it during the weekend, but I will do it next week.

Thanks,
Xavi

@ncrubin

This comment has been minimized.

Copy link
Collaborator

commented Jun 3, 2019

@xabomon bump on this? it would be great to get this in!

@xabomon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Hi Nick,

I'm really sorry for not being active on this lately.

I manage to narrow down that the test fail in python 2.7. I am suspicious that the fail is due to our customize error messages but I have seen code within OpenFermion with customize errors.

Currently I'm building an environment with 2.7 to see if I can reproduce the issue outside the test module.

Please, let me know if you have heard about similar issues before and how to address them.

Update: Found issue with customize error classes in python2, the super()init() does not work properly and one must use Exception.init() to initialize the inherent class.

Best,
Xavi

@xabomon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Hi @ncrubin ,
We have fixed the CI issue but now the coverage is failing.

As far as I understand, the coveralls counts how many new lines are used in the test module. We have tested all possible issue in the main functions, this implies that internal functions are indirectly tested. Do we have to also cover internal functions?

Thanks for the assistance.

Xavi

@xabomon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

I finally managed to fulfill CI and coverage.

Hopefully, we can now merge this PR.

Best,
Xavi

@ncrubin ncrubin added the cla: yes label Jun 5, 2019

@googlebot

This comment has been minimized.

Copy link

commented Jun 5, 2019

☹️ Sorry, but only Googlers may change the label cla: yes.

@googlebot googlebot removed the cla: yes label Jun 5, 2019

@ncrubin ncrubin added cla: yes and removed cla: no labels Jun 5, 2019

@googlebot googlebot removed the cla: yes label Jun 5, 2019

@googlebot

This comment has been minimized.

Copy link

commented Jun 5, 2019

☹️ Sorry, but only Googlers may change the label cla: yes.

@ncrubin

ncrubin approved these changes Jun 5, 2019

@googlebot googlebot added the cla: no label Jun 5, 2019

@ncrubin ncrubin added cla: yes and removed cla: no labels Jun 5, 2019

@googlebot

This comment has been minimized.

Copy link

commented Jun 5, 2019

☹️ Sorry, but only Googlers may change the label cla: yes.

@googlebot googlebot removed the cla: yes label Jun 5, 2019

@ncrubin ncrubin merged commit 188d54e into quantumlib:master Jun 5, 2019

2 of 3 checks passed

cla/google CLAs are signed, but unable to verify author consent
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0004%) to 99.625%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.