Support algebraic operations for BaseNode #358

jziub · 2019-10-31T21:42:12Z

This is to fix #292 .

googlebot · 2019-10-31T21:42:14Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.


        Add + operator for numpy backend.

googlebot · 2019-10-31T21:43:27Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

jziub · 2019-10-31T21:46:05Z

This is an implementation of addition in numpy backend. Will add other operations if this is the right direction.

For the returned node, does it need to be a copy of the left hand node? Or just a new node with the dangling edges?

Thenerdstation · 2019-10-31T23:47:43Z

Hm, that's a good question. Perhaps it would only make sense to support modifying the tensor in place, so we would only support a += b type operations.

@mganahl what are your thoughts?

mganahl · 2019-11-01T18:32:26Z

I think typically one would expect such an operation to return a new object, so I would say __add__ should return a new Node with new edges. We can still have __iadd__ as well, in which case everything except Node.tensor stays the same as it was.

jziub · 2019-11-01T19:01:18Z

Thanks for clarifying. Will return a brand new node then.

Thenerdstation · 2019-11-03T18:00:37Z

Thanks for the PR!

Other projects will take my priority right now, so I will review this in ~2 weeks.

Thenerdstation · 2019-11-03T18:01:32Z

Thanks for the PR!

Other projects will take priority for me on the short term, so I will review this ~2 weeks.

googlebot · 2019-11-04T04:50:03Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.


        add implementation to other backends

googlebot · 2019-11-04T04:54:07Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

jziub · 2019-11-04T04:56:24Z

No problem. Have the addition and subtraction implemented across backends.

Thenerdstation · 2019-11-12T00:15:25Z

tensornetwork/backends/shell/shell_backend.py

+    a = np.ones(tensor1.shape)
+    b = np.ones(tensor2.shape)
+    return ShellTensor((a + b).shape)
+
+  def sub(self, tensor1: Tensor, tensor2: Tensor) -> Tensor:
+    a = np.ones(tensor1.shape)
+    b = np.ones(tensor2.shape)


Not sure I agree with how this is implemented. This will blow up if the shapes are huge (which would be the main reason to use the shell backend in the first place).

I agree, just check if the shapes the tensors match and then create a ShellTensor directly

Changed as suggested. Added another unit test to check mismatch shape. Please take another look.


        address comments

mganahl · 2019-11-20T18:49:14Z

tensornetwork/backends/tensorflow/tensorflow_backend.py

@@ -83,7 +83,10 @@ def diag(self, tensor: Tensor) -> Tensor:
    return self.tf.linalg.diag(tensor)

  def convert_to_tensor(self, tensor: Tensor) -> Tensor:
-    result = self.tf.convert_to_tensor(tensor)
+    if isinstance(tensor, (float, int)):


what is this case covering?

Given a scalar, we need to specify the data type to create a tensor in tensorflow.

Just worried that explicit use of tf.float64 could lead to problems if other dtypes are e.g. tf.float32

hmm, the input is constrained to float and int in python type, could that protect tf.float64?

Can you tell me why you added this?

self.tf.convert_to_tensor(tensor) throws an error if the input is float/int.

OK, but where in the code do you need to pass a float or int to backed.convert_to_tensor?

Following functions call backed.convert_to_tensor. Test case is like np.ones((1, 2, 3)) + 2

def sub()
def add()

mganahl · 2019-11-20T18:50:22Z

tensornetwork/network_components.py

+      raise AttributeError("Please provide a valid tensor for this Node.")
+
+    if isinstance(other, BaseNode):
+      other_tensor = other.get_tensor()


I thought we had deprecated get_tensor()?

Replaced with tensor.

googlebot · 2019-11-22T01:47:50Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.


        Replace get_tensor()

googlebot · 2019-11-22T01:48:45Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

mganahl · 2019-11-22T02:26:51Z

tensornetwork/backends/tensorflow/tensorflow_backend_test.py

@@ -257,3 +257,33 @@ def test_multiply(a, b, expected):
  tensor2 = backend.convert_to_tensor(b)

  np.testing.assert_allclose(backend.multiply(tensor1, tensor2), expected)
+
+
+@pytest.mark.parametrize("a, b, expected",


please test this for all supported dtypes

Add test cases(a + b, b could be vector, float, or . int). Please take another look.

mganahl · 2019-11-22T02:28:01Z

tensornetwork/tests/network_test.py

@@ -537,3 +537,33 @@ def test_remove_node(backend):
  broken_edges_by_name, broken_edges_by_axis = tn.remove_node(b)
  assert broken_edges_by_name == {"0": a[0]}
  assert broken_edges_by_axis == {0: a[0]}
+
+
+@pytest.mark.parametrize("a, b, expected",


please parametrize tests over different dtypes

also check that dtypes are preserved (no implicit conversions)

Add test cases(a + b, b could be vector, float, or . int). Please take another look.


        add more test cases

googlebot added the cla: no label Oct 31, 2019

Add + operator for numpy backend.

Loading status checks…

0ee2a3b

jziub force-pushed the jziub:alg-ops branch from bbff216 to 0ee2a3b Oct 31, 2019

googlebot added cla: yes and removed cla: no labels Oct 31, 2019

MichaelMarien mentioned this pull request Nov 1, 2019

Scalar operator changes for Tensor on NumpyBackend #360

Closed

googlebot added cla: no and removed cla: yes labels Nov 4, 2019

add implementation to other backends

Loading status checks…

227fd29

jziub force-pushed the jziub:alg-ops branch from c3f2335 to 227fd29 Nov 4, 2019

googlebot added cla: yes and removed cla: no labels Nov 4, 2019

Thenerdstation reviewed Nov 12, 2019

View changes

address comments

Loading status checks…

1541c55

mganahl reviewed Nov 20, 2019

View changes

googlebot added cla: no and removed cla: yes labels Nov 22, 2019

Replace get_tensor()

Loading status checks…

992573a

jziub force-pushed the jziub:alg-ops branch from 759a895 to 992573a Nov 22, 2019

googlebot added cla: yes and removed cla: no labels Nov 22, 2019

mganahl reviewed Nov 22, 2019

View changes

add more test cases

Loading status checks…

792ca78

google / TensorNetwork

Support algebraic operations for BaseNode #358

Support algebraic operations for BaseNode #358

jziub commented Oct 31, 2019

googlebot commented Oct 31, 2019

googlebot commented Oct 31, 2019

jziub commented Oct 31, 2019

Thenerdstation commented Oct 31, 2019

mganahl commented Nov 1, 2019

jziub commented Nov 1, 2019

Thenerdstation commented Nov 3, 2019

Thenerdstation commented Nov 3, 2019

googlebot commented Nov 4, 2019

googlebot commented Nov 4, 2019

jziub commented Nov 4, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

googlebot commented Nov 22, 2019

googlebot commented Nov 22, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

google / TensorNetwork

Join GitHub today

Support algebraic operations for BaseNode #358

Support algebraic operations for BaseNode #358

Conversation

jziub commented Oct 31, 2019

googlebot commented Oct 31, 2019

googlebot commented Oct 31, 2019

jziub commented Oct 31, 2019

Thenerdstation commented Oct 31, 2019

mganahl commented Nov 1, 2019

jziub commented Nov 1, 2019

Thenerdstation commented Nov 3, 2019

Thenerdstation commented Nov 3, 2019

googlebot commented Nov 4, 2019

googlebot commented Nov 4, 2019

jziub commented Nov 4, 2019

This comment has been minimized.

Thenerdstation Nov 12, 2019 Collaborator

This comment has been minimized.

mganahl Nov 12, 2019 Collaborator

This comment has been minimized.

jziub Nov 13, 2019 Author Contributor

This comment has been minimized.

mganahl Nov 20, 2019 Collaborator

This comment has been minimized.

jziub Nov 22, 2019 Author Contributor

This comment has been minimized.

mganahl Nov 22, 2019 Collaborator

This comment has been minimized.

jziub Nov 22, 2019 Author Contributor

This comment has been minimized.

mganahl Nov 22, 2019 Collaborator

This comment has been minimized.

jziub Nov 22, 2019 Author Contributor

This comment has been minimized.

mganahl Nov 22, 2019 Collaborator

This comment has been minimized.

jziub Nov 22, 2019 Author Contributor

This comment has been minimized.

mganahl Nov 20, 2019 Collaborator

This comment has been minimized.

jziub Nov 22, 2019 Author Contributor

googlebot commented Nov 22, 2019

googlebot commented Nov 22, 2019

This comment has been minimized.

mganahl Nov 22, 2019 Collaborator

This comment has been minimized.

jziub Nov 22, 2019 • edited Author Contributor

This comment has been minimized.

mganahl Nov 22, 2019 Collaborator

This comment has been minimized.

mganahl Nov 22, 2019 Collaborator

This comment has been minimized.

jziub Nov 22, 2019 • edited Author Contributor

jziub Nov 22, 2019 •

edited

Author Contributor

jziub Nov 22, 2019 •

edited

Author Contributor