Add FermionOperator.zero/identity

[Strilanc]

These can be used in a few places in the code base that do sum/product aggregations. Instead of starting with a None seed and checking for it on every loop iteration, just start with one of these.

[Strilanc]

Rebased.

[damiansteiger]

This seems unnecessary to me. Why would you want this?

[Strilanc]

I'm going to use it in a future PR that cleans up the Fourier transform method. These methods also act as a signal that this class is a ring.

[babbush]

I am also a bit confused about this one. Does the normal identity operator (i.e. FermionOperator(())) not have this multiplicative property? Doesn't the zero operator (i.e. FermionOperator()) have the additive property?

[Strilanc]

It's more about giving them a name and calling out your intentions when using them. FermionOperator(()) is not very descriptive of intent, but FermionOperator.multiplicative_identity() is (even when you're not familiar with the behavior of the constructor).

It's also worth noting that the current constructor can't actually construct instances of the class that you can create via arithmetic, so its signature is likely to change to fix that. Code using the identity methods will not be touched by such a change.

[Strilanc]

I expanded this PR to include the sum and product aggregator methods I was going to add after.

[idk3]

+1 for additive and multiplicative identity. IMO it might be better to call them zero and identity. I agree with Damian that they're not totally necessary, but the advantage of simplicity and total clarity is great. During development the default FermionOperator init changed from returning identity to zero - this avoids confusion when changes like this happen, as well as more broadly. However, it isn't clear to me that sum and product don't mirror existing Python functions.

[Strilanc]

They don't mirror the existing functions for two reasons:

• If you try to use the built-in sum or product you'll end up left-multiplying or left-adding by an int, which causes an exception.
• The built-in constructs use __add__ and __mul__ instead of __iadd__ and __imul__, so these ones are much faster (and could be made even faster in the future if needed).

[babbush]

Sorry Ian and Craig, I still do not understand this. You seem to be suggesting that additive_identity() does something other than FermionOperator() but that is not true as far as I can tell. Likewise, FermionOperator(()) is clearly the same thing as the multiplicative identity. Furthermore, this is being introduced as a static method and I do not understand why. Craig, I don't understand your last comment at all about how this methods has any impact on the behavior of the built-in sum and product.

I am inclined to not accept this PR. There is something to be said for keeping the code around the core data structures minimal so that people don't need to learn a ton of new functions to use the library. FermionOperator() gives the zero operator which I think is very intuitive because you did not specify any terms. FermionOperator(()) gives identity because you specified the terms (no operators) but not the coefficients.

[babbush]

At the very least, the names should be changed. I can live with FermionOperator.zero() to create what you call additive_identity and FermionOperator.identity() for what you call multiplicative_identity().

[damiansteiger]

I also don't understand this sentence:

If you try to use the built-in sum or product you'll end up left-multiplying or left-adding by an int, which causes an exception.

Again I don't think the multiplicative and additive identity are necessary and hence we should not include them.
FermionOperator() should be clear that this creates an empty FermionOperator hence a multiplicative identity
While FermionOperator(()) is a bit harder to read, it still makes sense as you programmers should know the internal data structure (or learn it by heart). If that is not totally clear I would rather prefer to extend the string initializer to allow "I" for multiplicative identity:
FermionOperator((), 0.5) == FermionOperator("I", 0.5)
What do people think about this? I would be happy with no changes.

The built-in constructs use add and mul instead of iadd and imul, so these ones are much faster (and could be made even faster in the future if needed).

This is actually a false statement if you check the implementation (https://github.com/ProjectQ-Framework/FermiLib/blob/develop/src/fermilib/ops/_fermion_operator.py#L389) for __imul__. It cannot be done in-place as you cannot update the terms dictionary while looping over it and therefore it also creates a new dict as would __mul__. Hence, I don't see a point for sum and product

[babbush]

I agree with Damian very much here. I am going to close this PR.

[Strilanc]

FermionOperator() should be clear that this creates an empty FermionOperator hence a multiplicative identity

That's funny, because FermionOperator() actually creates a multiplicative zero not a multiplicative identity. This is a great demonstration of why these methods are useful. The behavior of constructors is too hard to predict. I agree with babbush that you probably just typo'd.

While FermionOperator(()) is a bit harder to read, it still makes sense as you programmers should know the internal data structure (or learn it by heart).

I agree that someone writing code should probably know this distinction, but I disagree that a reader will know it.

[saying that the built-in sum and product don't use iadd/imul] is actually a false statement if you check the implementation (https://github.com/ProjectQ-Framework/FermiLib/blob/develop/src/fermilib/ops/_fermion_operator.py#L389) [...]

Sorry for any confusion. I was referring to python's built-in sum method.

[babbush]

I think Damian just had a typo: he clearly identified FermionOperator() with the zero operator which is additive identity. I actually will reopen this PR. I don't really care strongly enough. I will let @jarrodmcc break the tie. Jarrod, please accept this PR or close it.

[Strilanc]

I'm fine with changing the method names. Also, it may be the case that adding the sum/product methods subsumes the usefulness of exposing the identities. So we can downgrade to just adding those two for now.

You seem to be suggesting that additive_identity() does something other than FermionOperator() but that is not true as far as I can tell.

You're right that it doesn't do anything differently, it does things more clearly. You're familiar with the code, so it feels natural to you that FermionOperator() is the zero element. And I think that part probably is clear, maybe.

FermionOperator(()) gives identity because you specified the terms (no operators) but not the coefficients.

FermionOperator(()) for the multiplicative identity definitely isn't a-priori clear. In fact I would have guessed that ``FermionOperator(())was identical toFermionOperator()`, because `FermionOperator` is a collection-of-collections-of-ladder-operators. Actually, I bet in a year that statement will be true and the proper way to create a multiplicative identity will be `FermionOperator({(): 1})`.

[Strilanc]

@babbush You're right that it's probably just a typo. Sorry @damiansteiger

[Strilanc]

Given the disagreement, I'll scale this back to just adding sum and product. Adding [WIP].

[idk3]

I disagree with scaling it back, I think sum and product are the unnecessary functions while the class methods (renamed zero and identity) are the ones that should be kept. @Strilanc's point that

I would have guessed that FermionOperator(()) was identical to FermionOperator()

is very to the point, considering that this is what that function did only a few weeks ago. Having FermionOperator.zero() and FermionOperator.identity() instead of FermionOperator() and FermionOperator(()) makes the difference immediately clear to a first-time reader going through the code and avoids any potential for confusion / ambiguity.

[Strilanc]

Hm. Okay. Then I'll... rename the methods and that's sufficient?

[babbush]

Jarrod is being too slow. I will side with Ian here. He spent a long time writing tests for the code and said this confused him at times. So, sure just change to .zero() and .identity() and I'll accept.

[Strilanc]

Renaming done.

[Strilanc]

@damiansteiger You need to dismiss your review if you're okay with this being merged.

[damiansteiger]

@babbush and @Strilanc
Why do we need sum and product?

[thomashaener]

@babbush For what it's worth, I agree with Ian saying

[...] I think sum and product are the unnecessary functions while the class methods (renamed zero and identity) are the ones that should be kept.

[thomashaener]

Sorry about this, I hit the wrong button :)

[babbush]

I agree with you guys: I feel we do not need sum and product. This PR got really large. I have gotten used to the idea of .zero() and .identity() and kind of like them now. Even though I do wish to keep the functionality of FermionOperator() and FermionOperator(()).

[Strilanc]

I suspect Sum/Product will reappear in some other PR, but I've cut them from this one.

[damiansteiger]

👍 Thanks Craig