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

Define MajoranaOperator #501

Merged
merged 25 commits into from Feb 28, 2019

Conversation

Projects
None yet
3 participants
@kevinsung
Copy link
Collaborator

kevinsung commented Jan 9, 2019

Fixes #458

kevinsung added some commits Jan 8, 2019

@kevinsung kevinsung requested a review from babbush Jan 9, 2019

@googlebot googlebot added the cla: yes label Jan 9, 2019

@kevinsung

This comment has been minimized.

Copy link
Collaborator Author

kevinsung commented Jan 9, 2019

Also, I still need to add a docstring.

kevinsung added some commits Jan 9, 2019

doc
@kevinsung

This comment has been minimized.

Copy link
Collaborator Author

kevinsung commented Jan 9, 2019

@babbush ready for review.

kevinsung added some commits Jan 9, 2019

cov
@ncrubin
Copy link
Collaborator

ncrubin left a comment

I'm curious why the choice for not extending SymbolicOperator base class? Is it because SymbolicOperator requires an "action" which does not carry real meaning for Majorana's? Or was there some other reason like the algebra and operations are sufficiently different than other things extending SymbolicOperator?

You might get some things for free if extending SymbolicOperator like string initialization if _simplify gets implemented.

I'll add a more detailed review and check for correctness shortly.

@kevinsung

This comment has been minimized.

Copy link
Collaborator Author

kevinsung commented Jan 13, 2019

Yes, the main reason is that there is no action, so it's more natural to store the terms simply as tuples rather than tuples of tuples. It's possible that in the future it could become a subclass of SymbolicOperator, but in my opinion that requires some pretty fundamental changes to SymbolicOperator. In particular, I think the way to do that would be to define classes to represent the terms themselves and to implement term multiplication within those classes rather than in SymbolicOperator. However, that is a long-term and potentially controversial change.

Args:
term (Tuple[int]): The indices of a Majorana operator term
to start off with

This comment has been minimized.

@ncrubin

ncrubin Jan 17, 2019

Collaborator

text needs to be aligned with 'The' so it renders properly.

This comment has been minimized.

@kevinsung

kevinsung Feb 27, 2019

Author Collaborator

That's not true. We do it like here elsewhere and it renders fine.

def commutes_with(self, other):
"""Test commutation with another MajoranaOperator"""
if not isinstance(other, type(self)):
return NotImplemented

This comment has been minimized.

@ncrubin

ncrubin Jan 17, 2019

Collaborator

Coveralls decrease is because this pathway is not tested. A small addition of with pytest.raises(NotImplemented) should test this path to make sure the error is thrown.

This comment has been minimized.

@kevinsung

kevinsung Feb 27, 2019

Author Collaborator

Done


def __iadd__(self, other):
if not isinstance(other, type(self)):
return NotImplemented

This comment has been minimized.

@ncrubin

ncrubin Jan 17, 2019

Collaborator

Ditto on the coverage decrease

This comment has been minimized.

@kevinsung

kevinsung Feb 27, 2019

Author Collaborator

Done


def __add__(self, other):
if not isinstance(other, type(self)):
return NotImplemented

This comment has been minimized.

@ncrubin

ncrubin Jan 17, 2019

Collaborator

ditto on the coverage decrease

This comment has been minimized.

@ncrubin

ncrubin Jan 17, 2019

Collaborator

actually just the same for all below

This comment has been minimized.

@kevinsung

kevinsung Feb 27, 2019

Author Collaborator

Done

return merged_term, (left_parity + right_parity + merge_parity) % 2


def _merge_majorana_terms(left_term, right_term):

This comment has been minimized.

@ncrubin

ncrubin Feb 14, 2019

Collaborator

This needs a docstring

This comment has been minimized.

@kevinsung

kevinsung Feb 27, 2019

Author Collaborator

Done.

kevinsung added some commits Feb 27, 2019

@kevinsung kevinsung merged commit 6fb280c into quantumlib:master Feb 28, 2019

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.007%) to 99.606%
Details

@kevinsung kevinsung deleted the kevinsung:majorana_operator branch Feb 28, 2019

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.