Add SymbolicOperator and tests #194

kevinsung · 2018-01-30T07:21:37Z

I couldn't add commits to #191 because I don't have write access to the main repo, so I'm replacing it with this PR. Some notes on my implementation:

I added four attributes that all subclasses need to define; see their descriptions in the docstring for SymbolicOperator. So far, the only thing I used the attribute different_indices_commute for is to see if we can reorder the factors in terms so that the indices are in ascending order. To get a functioning subclass it's enough to set these four attributes; I removed the uses of abstract base class methods (from the module abc) which were introduced in Added SymbolicOperator Class #191.
The implementation of __str__ preserves the current string representations for FermionOperators, but would slightly alter those of QubitOperators. Namely, operators would be surrounded in square brackets, and the identity term would be represented as [] instead of I. For instance, we would have

2.0 [] +
0.6 [Y1 Y3 Z8] +
0.5 [X1 Y3 Z8]

instead of

2.0 I +
0.6 Y1 Y3 Z8 +
0.5 X1 Y3 Z8

Of course, we can override this behavior but using the unified implementation will make it possible for #177 to be implemented only in SymbolicOperator.

On that note, a unified implementation of Expand string initializations #177 now amounts to writing the method _long_string_init which I left unimplemented.
All the tests are pretty much straight from _fermion_operator_test.py and _qubit_operator_test.py. In the test file, the tests for DummyOperator1 were from _fermion_operator_test.py and the tests for DummyOperator2 were from _qubit_operator_test.py (modified to remove the dependence on pytest).
I replaced all uses of SymbolicOperatorError with ValueError because I couldn't tell when one should be used over the other.
It appears that most of the work that will go into defining a subclass is in overriding __imul__.
I started calling the things that terms are made of "factors", but then realized later on that these were sometimes called "local operators" which is perhaps a better term. I still document them as "factors" but we can change this.

googlebot · 2018-01-30T07:21:39Z

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 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.

babbush

Thanks Kevin, this PR is looking great. I am surprised though that you weren't able to checkout @bgimby's PR. If you clone his fork, which is public at (https://github.com/bgimby/OpenFermion) then you should be able to checkout his feature branch. But in any event, I think it is fine to merge this from your fork. But do try to work off of @bgimby's fork for the next PR so that he gets some credit in the "contributors" section for the lines of code contributed.

Aside from a comment about Latin abbreviations, I think your PR is basically good to go. I have a couple questions though because I want to completely understand the plan before moving forward.

Note that the performance of FermionOperator and QubitOperator is very important to us. It might be a good idea to check performance by looking at the "performance_benchmarks" in the examples folder to make sure we aren't slowing down these classes. One important question: did you alter any of the magic methods like add, mul, etc. in any way that could possibly effect performance?

If not, I think we are basically ready to merge.

babbush · 2018-01-30T19:30:42Z

src/openfermion/ops/_symbolic_operator.py

+    SymbolicOperators of the same type can be added or multiplied together.
+
+    Attributes:
+        actions (tuple): A tuple of objects representing the possible actions.


I wonder if it would be better if everything other than the terms attribute were a private attribute that is accessed with getters/setters. I am not necessarily of that belief, but it is worth thinking about. @jarrodmcc do you have any thoughts?

babbush · 2018-01-30T19:33:49Z

src/openfermion/ops/_symbolic_operator.py

+        # Add the term to the dictionary
+        self.terms[term] = coefficient
+
+    def _long_string_init(self, term, coefficient):


Is there a reason why this needs to be implemented differently for FermionOperator and QubitOperator? Otherwise, I feel we should implement the method now.

No, there is no reason. It's just that @max-radin said he would do it at #177 so I figured I'd leave it to him. Perhaps we should post there asking if he's still willing to do it? I figured we can leave it unimplemented here and then post about it there after merging. In any case I'd be happy to write this.

Yeah let's ask him about that.

babbush · 2018-01-30T19:39:25Z

src/openfermion/ops/_symbolic_operator.py

+    def _parse_sequence(self, term):
+        """Parse a term given as a sequence type (i.e., list, tuple, etc.).
+
+        i.e. For QubitOperator:


I think you mean "e.g." not "i.e."

Ok I will change these.

babbush · 2018-01-30T19:40:13Z

src/openfermion/ops/_symbolic_operator.py

+    def _parse_string(self, term):
+        """Parse a term given as a string.
+
+        i.e. For FermionOperator:


You mean "e.g." not "i.e."

babbush · 2018-01-30T19:44:01Z

src/openfermion/ops/_symbolic_operator.py

+    def __repr__(self):
+        return str(self)
+
+    def __imul__(self, multiplier):


So is the idea that for QubitOperator we will overwrite this class? Because in that case there is a more optimized way to multiply which is based on knowledge of the fact that the QubitOperator terms are sorted by tensor factor. I think that is fine, I am just trying to understand.

Yes, the idea is that QubitOperator will overwrite this method using its existing implementation. This is just the most general multiplication method that doesn't take into account any additional structure in the space of operators.

kevinsung · 2018-01-31T02:35:13Z

Actually, I did work off of @bgimby's fork. I just couldn't add commits to the PR he created because that required me to push directly to his branch, which I don't have write access to. Creating a PR gives write access to maintainers of the main repository. The first two commits of this PR belong to him so I think he will get credit on the Contributor's page.

@bgimby was the one who ported most of the magic methods. It looks to me that they are unmodified; do you have anything else to say about that @bgimby? I made sure that __imul__ and __iadd__ work exactly the same. I promise to check the performance benchmarks after I complete the inheritance structure.

bgimby · 2018-01-31T02:55:15Z

I did not modify any of the magic methods. It's possible that there is extra overhead in calling a base class's method from a subclass, though I'm not sure.

If there are any performance losses, or indeed any performance issues I'll be happy to work on them when I have the time- I like working on optimization, though I haven't had a chance to do much in python.

babbush · 2018-01-31T03:00:50Z

In that case, I think we're pretty much good to go. Kevin, let me know when we're ready. Also, @kevinsung did you have a chance to figure out why the docs were failing from your last PR? If you have any idea it would be good to also fix that in this PR.

kevinsung · 2018-01-31T03:09:28Z

@babbush I think this PR is ready. No, I couldn't figure out why the docs are failing; I tried building the docs on my computer and did not get any errors or warnings besides the h5py one mentioned in #175 . Currently I have no idea how to diagnose the problem but I'll keep it in mind.

babbush · 2018-01-31T03:35:14Z

src/openfermion/ops/_symbolic_operator.py

+        # Add the term to the dictionary
+        self.terms[term] = coefficient
+
+    def _long_string_init(self, term, coefficient):


Yeah let's ask him about that.

babbush · 2018-01-31T04:02:14Z

Somehow the docs are unbroken. I think it was some glitch in read-the-docs.

* Added SymbolicOperator Class * PR fixes * documentation and init * add test file * add default implementation for imul * add action attributes * write str and parse_sequence * steal tests from FermionOperator and QubitOperator * remove uses of SymbolicOperatorError * edit docs * lint * remove some whitespace * make _parse_string self-contained * add duplicate terms multiplication test * remove extraneous imports * change iadd to use EQ_TOLERANCE * correct Latin abbreviations

bgimby and others added 10 commits January 24, 2018 01:31

Added SymbolicOperator Class

b63452b

PR fixes

07e4c36

documentation and init

e33958e

add test file

d4b41a5

add default implementation for imul

1269398

add action attributes

3a269aa

write str and parse_sequence

dad12e5

steal tests from FermionOperator and QubitOperator

d58cc96

remove uses of SymbolicOperatorError

f7f00ab

edit docs

ac93e94

kevinsung added 6 commits January 29, 2018 23:28

lint

541fcf5

remove some whitespace

769ab6e

make _parse_string self-contained

6c4bb03

Merge branch 'master' into add_symbolic_operator

1fbbf6b

add duplicate terms multiplication test

68af164

remove extraneous imports

c1bb06f

babbush suggested changes Jan 30, 2018

View reviewed changes

change iadd to use EQ_TOLERANCE

1054db1

correct Latin abbreviations

e8a28cc

babbush approved these changes Jan 31, 2018

View reviewed changes

babbush merged commit aac09d0 into quantumlib:master Jan 31, 2018

kevinsung deleted the add_symbolic_operator branch January 31, 2018 04:47

kevinsung mentioned this pull request Jan 31, 2018

Expand string initializations #177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SymbolicOperator and tests #194

Add SymbolicOperator and tests #194

kevinsung commented Jan 30, 2018 •

edited

Loading

googlebot commented Jan 30, 2018

babbush left a comment

babbush Jan 30, 2018

babbush Jan 30, 2018

kevinsung Jan 31, 2018 •

edited

Loading

babbush Jan 31, 2018

babbush Jan 30, 2018

kevinsung Jan 31, 2018

babbush Jan 30, 2018

babbush Jan 30, 2018

kevinsung Jan 31, 2018 •

edited

Loading

kevinsung commented Jan 31, 2018

bgimby commented Jan 31, 2018

babbush commented Jan 31, 2018

kevinsung commented Jan 31, 2018

babbush Jan 31, 2018

babbush commented Jan 31, 2018

Add SymbolicOperator and tests #194

Add SymbolicOperator and tests #194

Conversation

kevinsung commented Jan 30, 2018 • edited Loading

googlebot commented Jan 30, 2018

babbush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinsung Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinsung Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

kevinsung commented Jan 31, 2018

bgimby commented Jan 31, 2018

babbush commented Jan 31, 2018

kevinsung commented Jan 31, 2018

Choose a reason for hiding this comment

babbush commented Jan 31, 2018

kevinsung commented Jan 30, 2018 •

edited

Loading

kevinsung Jan 31, 2018 •

edited

Loading

kevinsung Jan 31, 2018 •

edited

Loading