Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/openfermion/contrib/representability/_multitensor.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def __iter__(self):


class MultiTensor(object):
def __init__(self, tensors, dual_basis=DualBasis()):
def __init__(self, tensors, dual_basis=None):
"""
A collection of tensor objects with maps from name to tensor

Expand All @@ -46,6 +46,8 @@ def __init__(self, tensors, dual_basis=DualBasis()):
self.off_set_map = self.make_offset_dict(self.tensors)

# An iterable object that provides access to the dual basis elements
if dual_basis is None:
dual_basis = DualBasis()
self.dual_basis = dual_basis
self.vec_dim = sum([vec.size for vec in self.tensors])

Expand Down Expand Up @@ -81,8 +83,7 @@ def add_dual_elements(self, dual_element):
if not isinstance(dual_element, DualBasisElement):
raise TypeError("dual_element variable needs to be a DualBasisElement type")

# we should extend TMap to add
self.dual_basis.elements.extend(dual_element)
self.dual_basis.elements.append(dual_element)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can you update or remove the comment above this.

Are we sure add is the intended operation here?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The function name is plural but the argument name is singular. The docstring says "add" but the inline comment says "extend". maybe this was originally an extend-type operation and was changed to an append-type?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

can you update or remove the comment above this.

Ah, yes, good point. Thanks for catching that. Fixed in the next push.

The function name is plural but the argument name is singular. The docstring says "add" but the inline comment says "extend". maybe this was originally an extend-type operation and was changed to an append-type?

Yeah, something probably happened there. I also think the bug was never caught before now because the tests in _multitensor_test.py were incomplete. (I only ran into the problem because I was working on getting parallel Pytest working, which revealed the problem of initializing MultiTensor with a mutable object, which led to more poking around.)

Here's what's happening:

  1. DualBasisElement objects are iterable because they implement the __iter__() magic method (c.f. _dualbasis.py). Each call to that method returns a tuple of 3 items.

  2. In Python, doing somelist.extend(someobject) invokes the __iter__ method on someobject. On a DualBasisElement object, doing that ends up adding a tuple of 3 items to somelist. That's in contrast with the operation somelist.append(someobject), which puts the DualBasisElement object itself on somelist.

  3. The previous _multitensor_test.py only tested adding an item to the list dual_basis.elements, and never tested using the results. The method synthesize_dual_basis() eventually calls the synthesize_element() method on each item on the list, and therein lies the problem: synthesize_element() does a loop like this:

    for tlabel, velement, coeff in element:

    That works as expected if the values on the elements list in DualBasis are actual DualBasisElement objects because their __iter__() method will get called in the for loop (yielding the 3 items above), but that won't happen if the values on the elements list are tuples.


def synthesize_dual_basis(self):
"""
Expand All @@ -93,7 +94,7 @@ def synthesize_dual_basis(self):

:returns: sparse matrix
"""
# go throught the dual basis list and synthesize each element
# go through the dual basis list and synthesize each element
dual_row_indices = []
dual_col_indices = []
dual_data_values = []
Expand Down
27 changes: 27 additions & 0 deletions src/openfermion/contrib/representability/_multitensor_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,33 @@ def test_add_dualelement():
mt.add_dual_elements(dbe)
assert len(mt.dual_basis) == 1

dbe2 = DualBasisElement()
dbe2.add_element('b', (0, 1, 2), 5)
mt.add_dual_elements(dbe2)
assert len(mt.dual_basis) == 2

A, bias, scalar = mt.synthesize_dual_basis()
assert A.shape[0] == 2
# Verify that the elements are correctly added as DualBasisElements
# and not as their internal tuples (which would cause synthesis to fail).
assert isinstance(mt.dual_basis[0], DualBasisElement)
assert isinstance(mt.dual_basis[1], DualBasisElement)


def test_multitensor_init_isolation():
# Test that different MultiTensor instances don't share the same dual_basis.
a = np.random.random((2, 2))
at = Tensor(tensor=a, name='a')
mt1 = MultiTensor([at])
mt2 = MultiTensor([at])

dbe = DualBasisElement()
dbe.add_element('a', (0, 0), 1.0)
mt1.add_dual_elements(dbe)

assert len(mt1.dual_basis) == 1
assert len(mt2.dual_basis) == 0


def test_synthesis_element():
a = np.random.random((5, 5))
Expand Down