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.

Sign up for GitHub

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

Jump to bottom

Expose inline_sum #330

Merged
merged 1 commit into from May 3, 2018
Merged

Expose inline_sum #330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 6 additions & 18 deletions src/openfermion/transforms/_bravyi_kitaev.py
View file
Open in desktop
Expand Up @@ -13,7 +13,7 @@
"""Bravyi-Kitaev transform on fermionic operators."""

from openfermion.ops import QubitOperator
from openfermion.utils import count_qubits
from openfermion.utils import count_qubits, inline_sum


def bravyi_kitaev(operator, n_qubits=None):
Expand Down Expand Up @@ -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())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use the zero() and identity() class methods? I find it a little hard to distinguish () and (()) with a quick look

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@idk3 idk3 May 3, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@kevinsung kevinsung May 3, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



def _update_set(index, n_qubits):
Expand Down Expand Up @@ -121,8 +121,9 @@ def _transform_operator_term(term, coefficient, n_qubits):
_transform_ladder_operator(ladder_operator, n_qubits)
for ladder_operator in term
)
return inline_product(seed=QubitOperator((), coefficient),
factors=transformed_ladder_ops)
return inline_product(factors=transformed_ladder_ops,
seed=QubitOperator((), coefficient))



def _transform_ladder_operator(ladder_operator, n_qubits):
Expand Down Expand Up @@ -162,20 +163,7 @@ def _transform_ladder_operator(ladder_operator, n_qubits):
return transformed_operator


def inline_sum(seed, summands):
"""Computes a sum, using the __iadd__ operator.
Args:
seed (T): The starting total. The zero value.
summands (iterable[T]): Values to add (with +=) into the total.
Returns:
T: The result of adding all the factors into the zero value.
"""
for r in summands:
seed += r
return seed


def inline_product(seed, factors):
def inline_product(factors, seed):
"""Computes a product, using the __imul__ operator.
Args:
seed (T): The starting total. The unit value.
Expand Down
34 changes: 5 additions & 29 deletions src/openfermion/transforms/_bravyi_kitaev_tree.py
View file
Open in desktop
Expand Up @@ -14,6 +14,8 @@
from __future__ import absolute_import

from openfermion.ops import QubitOperator
from openfermion.utils import inline_sum
from openfermion.transforms._bravyi_kitaev import inline_product
from openfermion.transforms._fenwick_tree import FenwickTree


Expand Down Expand Up @@ -59,7 +61,7 @@ def bravyi_kitaev_tree(operator, n_qubits=None):
fenwick_tree=fenwick_tree)
for term in operator.terms
)
return inline_sum(seed=QubitOperator(), summands=transformed_terms)
return inline_sum(summands=transformed_terms, seed=QubitOperator())


def _transform_operator_term(term, coefficient, fenwick_tree):
Expand All @@ -78,8 +80,8 @@ def _transform_operator_term(term, coefficient, fenwick_tree):
_transform_ladder_operator(ladder_operator, fenwick_tree)
for ladder_operator in term
)
return inline_product(seed=QubitOperator((), coefficient),
factors=transformed_ladder_ops)
return inline_product(factors=transformed_ladder_ops,
seed=QubitOperator((), coefficient))


def _transform_ladder_operator(ladder_operator, fenwick_tree):
Expand Down Expand Up @@ -122,29 +124,3 @@ def _transform_ladder_operator(ladder_operator, fenwick_tree):
0.5)

return c_majorana_component + d_majorana_component


def inline_sum(seed, summands):
"""Computes a sum, using the __iadd__ operator.
Args:
seed (T): The starting total. The zero value.
summands (iterable[T]): Values to add (with +=) into the total.
Returns:
T: The result of adding all the factors into the zero value.
"""
for r in summands:
seed += r
return seed


def inline_product(seed, factors):
"""Computes a product, using the __imul__ operator.
Args:
seed (T): The starting total. The unit value.
factors (iterable[T]): Values to multiply (with *=) into the total.
Returns:
T: The result of multiplying all the factors into the unit value.
"""
for r in factors:
seed *= r
return seed
3 changes: 2 additions & 1 deletion src/openfermion/utils/__init__.py
View file
Open in desktop
Expand Up @@ -23,7 +23,8 @@

from ._operator_utils import (count_qubits, eigenspectrum, fourier_transform,
freeze_orbitals, get_file_path,
hermitian_conjugated, inverse_fourier_transform,
hermitian_conjugated, inline_sum,
inverse_fourier_transform,
is_hermitian, is_identity, prune_unused_indices,
reorder, up_then_down,
load_operator, save_operator)
Expand Down
13 changes: 13 additions & 0 deletions src/openfermion/utils/_operator_utils.py
View file
Open in desktop
Expand Up @@ -32,6 +32,19 @@ class OperatorUtilsError(Exception):
pass


def inline_sum(summands, seed):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This argument order is way better. It would be even nicer if the seeds defaulted to additive and multiplicative identities IMO!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually wait, is there a nice way to do this including both Fermion and QubitOperators?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, oh well, I'll approve

"""Computes a sum, using the __iadd__ operator.
Args:
seed (T): The starting total. The zero value.
summands (iterable[T]): Values to add (with +=) into the total.
Returns:
T: The result of adding all the factors into the zero value.
"""
for r in summands:
seed += r
return seed


def freeze_orbitals(fermion_operator, occupied, unoccupied=None, prune=True):
"""Fix some orbitals to be occupied and others unoccupied.

Expand Down