QubitOperator and FermionOperator should inherit from same parent

[babbush]

In a very early version of this library (known then as OpenPQRS), we had a parent class from which both QubitOperator and FermionOperator inherited. This was an obvious design choice because the classes are extremely similar and differ in only a few places. However, when making FermiLib (the predecessor to OpenFermion) we decided that the QubitOperator class should be embedded in ProjectQ and so we removed this inheritance structure. But in OpenFermion, we have again moved both QubitOperator and FermionOperator into the same library.

There are many advantages to having both of these classes inherit from the same parent. For one thing, we'd need many fewer tests to cover the same functionality. More importantly, there are a variety of methods I would like to add to these classes but it would be annoying to need to add essentially the same function to both. For instance, I would like to add the abs method to both classes which returns the L1 norm of the operators by summing the absolute value of the coefficients. This should be added to the parent class. I think the parent should be called SymbolicOperator.

This is not an easy change it will likely take significant effort. However, fortunately, one will likely only need to change the files which define the QubitOperator and FermionOperator and test them since all other parts of the library will still import QubitOperator and FermionOperator the same.

[idk3]

As a smaller part of this - FermionOperator has the static methods identity and zero, but QubitOperator doesn't have these methods.

[bgimby]

Hey, I was thinking of working on this issue.

Reading through the definitions of FermionOperator and QubitOperator, I think once this is completed, behavior of compress should be changed to remove small real parts of complex coefficients, as it does for small imaginary coefficients. At present, it would reduce 1+i*1e-15 to 1, but leave i+1e-15 unchanged, which I don't see a reason for.

Also, compress may be able to be done in place to save on unnecessary construction of new terms, although it may not be a bottleneck in any common applications, so I'm not sure if it'd be worth it.

Anyway, I'll get started on implementing a SymbolicOperator base class and making the others inherit from it.

[babbush]

Awesome! I agree completely with you that compress should also remove the small real components.

If you'd like, email me at my github username @google.com and I can send you some very old code that might be helpful. As I said in comment, in the early days of the code (before this GitRepo), we did have things setup this way. So I do have some old code implementing the SymbolicOperator that also has a bunch of tests that come with it. Of course, a number of things have changed since then but it still might be useful.

[kevinsung]

@idk3 What do you think about removing those static methods completely? I think our code would look nicer if there's only one way to initialize the zero and identity operator.

[babbush]

I brought up that same suggestion once Kevin and I think Ian and Craig pushed back. @idk3 @Strilanc can you remind us why you think they are necessary?

[idk3]

IMO the static methods are the only definitive way to initialize zero and identity, and FermionOperator() and FermionOperator(()) should be avoided where possible.

My basic problem is that FermionOperator() and FermionOperator(()) are ambiguous, to the point that they actually used to do the opposite of what they do now. The static methods make the distinction totally clear, both for code readability (no confusion about whether something is zero or identity) as well as for flexibility for future changes (should the initializer change again, the only thing we have to change is the static method, rather than literally every file in OpenFermion, as I had to do last time the definition changed ;)).

@Strilanc might have other reasons beyond these.

[kevinsung]

Hmm I see your concerns @idk3 . I guess what I'm proposing is that we agree here that the meaning of FermionOperator() and FermionOperator(()) shouldn't be changed in the future. Their current meanings have always made a lot of sense to me, and I'm pretty sure I'd be strongly opposed to any future change to them. In general, initializing a class instance with no arguments means "do the bare minimum amount of work", in this case meaning initialize an empty terms dictionary, while initializing with the empty tuple () as an argument implies that the empty tuple is a valid term, and there really is no other sensible choice for representing the constant shift (identity matrix) term.

[bgimby]

Hi guys, quick update on where I'm at. I've changed the QubitOperator and FermionOperator classes to inherit from SymbolicOperator and moved over all functions with identical behavior between them into _symbolic_operator.py. So far, all tests continue to pass and existing tests give 100% coverage of _symbolic_operator.py.

Up next will be messing with behavior a bit (adding static methods zero and identity to QubitOperator, changing compress as discussed above, adding abs).

After that will be trying to further reduce code duplication (moving identical code blocks into helper functions in SymbolicOperator etc), linting, writing documentation, and removing redundant test/adding new ones as needed.

Feel free to check out the changes in my fork.

Edit: For now, zero and identity have been changed to class methods in SymbolicOperator. This will give an ugly error when trying to call SymbolicOperator.zero() or SymbolicOperator.identity(). I think that this is not an issue unless we plan to expose SymbolicOperator to users, which I can't see a reason for at the moment.

[babbush]

This sounds like great progress. The first thing I will mention though is that you might want to check in the SymbolicOperator class once it is essentially "done". After that, you can check in the change that alters the FermionOperator and QubitOperator classes to inherit from the SymbolicOperator class. I suggest doing things this way to keep the pull requests manageable. This is going to be a big one and I anticipate it being rather confusing if you try to merge everything at once. This also gives us a nice forum to provide feedback in the context of the actual code (easier than flipping back and forth to look at your fork).

You asked me an interesting question offline which I would like to repeat and address here since I don't have a great solution but others might have input. You wrote:

If I understand the documentation generation correctly, it basically formats nicely the comments from the code itself, reading which class/function the documentation is referring to from the code itself. So in the current release, QubitOperator and FermionOperator have a lot of overlapping documentation. In my fork, many of the overlapping functions are moved into SymbolicOperator, so I think they will be documented under SymbolicOperator and not appear under FermionOperator and QubitOperator. I think this might be difficult to follow, especially if it isn't clear when constructing Qubit or Fermion operators that they both inherit from SymbolicOperator. I'm not exactly sure how the documentation process works, so I'd like to hear your thoughts.

The ideal solution would be if we had some way for the documentation to inherit as well! But I don't think that can be done. I suppose my recommendation would just be to make it very clear all over the FermionOperator / QubitOperator documentation that those classes inherit a lot of functionality. One should be able to then click a link in the web docs to see the parent class documentation.

[kevinsung]

With the change I proposed in #190 it would be made clear in the automatically generated online documentation that FermionOperator and QubitOperator inherit from SymbolicOperator.

[bgimby]

All right, I've submitted 2 separate PRs, one only containing the SymbolicOperator declaration file and another containing only the changes to QubitOperator and FermionOperator. Of course, the latter will fail tests right now, but I wanted to make the code simple to get to. Once the first one is accepted I can update the second one to include the SymbolicOperator declaration.

I did notice a potential issue: in __iadd__, the QubitOperator and FermionOperator differ in that the QubitOperator class deletes all terms in the summation that are identically zero, while the FermionOperator implementation deletes all terms whose absolute value is below EQ_TOLERANCE. I initially used the version with EQ_TOLERANCE, but with EQ_TOLERANCE = 1e-12, this caused the test JelliumTest.test_coefficients to fail. This test would pass with a lower tolerance, but a lower tolerance would cause other tests to fail due to assertAlmostEqual and friends being more strict. I'm not familiar enough with JelliumTest to tell if this is intended behavior or not, so I'm not sure what the solution should be.

For now, __iadd__ only deletes terms that are identically zero, so the behavior of FermionOperator will be slightly different. If this is unwanted, I can specialize the implementation of __iadd__.

[idk3]

@bgimby nice catch in JelliumTest! I think that explains an issue I was having on another PR.

[kevinsung]

@bgimby I noticed your change to __iadd__ and changed it back in my last PR, without realizing that you did it for this reason. However, on my computer the JelliumTest still passes. Indeed, it must also have passed on the coveralls server because otherwise we couldn't have merged the PR.

[bgimby]

It passes tests because in your merged PR the behavior was overridden by the definition in _qubit_operator.py.

OpenFermion/src/openfermion/ops/_qubit_operator.py

Line 398 in aac09d0

if abs(addend.terms[term] + self.terms[term]) > 0.:

In the version of PR #192 that removed the QubitOperator definition and left the SymbolicOperator definition as it is now, the travis tests failed.

checking out this commit or checking out the PR #192 branch and manually changing the behavior back should reproduce the bug.

[kevinsung]

Ah I see. Hmm we'll need to come up with a better solution than fudging with EQ_TOLERANCE...

[bgimby]

IMO the best solution is to only remove identically zero terms in functions that do not explicitly state that they remove small terms (like compress).

The user probably has a better idea than us when it is safe to remove or reduce small terms.

So we can either only remove identically zero terms in __iadd__ and friends, or mention in the documentation that small terms will be removed and then mess with EQ_TOLERANCE in the jellium test.

[babbush]

Closed by #192

We should perhaps also write something about this in the arXiv paper...