Expose inline_sum #330
Expose inline_sum #330
Conversation
| @@ -53,7 +53,7 @@ def bravyi_kitaev(operator, n_qubits=None): | |||
| n_qubits=n_qubits) | |||
| for term in operator.terms | |||
| ) | |||
| return inline_sum(seed=QubitOperator(), summands=transformed_terms) | |||
| return inline_sum(summands=transformed_terms, seed=QubitOperator()) | |||
There was a problem hiding this comment.
Could you use the zero() and identity() class methods? I find it a little hard to distinguish () and (()) with a quick look
There was a problem hiding this comment.
Sorry, I disagree with you. I strongly believe that QubitOperator() is the nicest way to initialize the zero operator in all situations where it's possible.
There was a problem hiding this comment.
Here it appears as QubitOperator()), so it's hard to read, vs zero which is impossible to mistake for one (e: identity).
I try to think of this from the perspective of a new user - they shouldn't have to think about what the default coefficient / initialization is. identity and zero make the difference 100% obvious. In the past (()) and () used to do the opposite thing as well so this is plainly not an intuitive difference. Why are you against it?
There was a problem hiding this comment.
I wasn't around when (()) and () meant the opposite, but you guys must have changed it because you realized it is totally inconsistent with the definition of the initialization function,
def __init__(self, term=None, coefficient=1.):
The identity is a term that has a coefficient, and there is no reasonable choice of tuple to represent it other than the empty tuple (). On the other hand, the zero operator represents the lack of any terms. Since the zero operator has no terms but the identity does have a term, initialization with no arguments must mean the zero operator. This is consistent with default initializers in general, which generally mean "do the bare minimum to instantiate a class". In this case, the bare minimum is to create a terms dictionary and leave it empty.
My way is more "pythonic" and I expect new users to pick up on the distinction very quickly.
| @@ -32,6 +32,19 @@ class OperatorUtilsError(Exception): | |||
| pass | |||
|
|
|||
|
|
|||
| def inline_sum(summands, seed): | |||
There was a problem hiding this comment.
This argument order is way better. It would be even nicer if the seeds defaulted to additive and multiplicative identities IMO!
There was a problem hiding this comment.
Actually wait, is there a nice way to do this including both Fermion and QubitOperators?
There was a problem hiding this comment.
You could get one of the summands and use the class method. That wouldn't be nice, though, because summands is an iterable and not a sequence.
Using this function is much more efficient than adding a list of SymbolicOperators using the built-in
sum. Previously it was hidden inbravyi_kitaev.py. Changed the argument order to match up with the built-insum.Also changed the argument order of
inline_productbut did not expose it.