-
Notifications
You must be signed in to change notification settings - Fork 117
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.
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
Add support for Tensorflow SparseTensors: merging layers. #925
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #925 +/- ##
===========================================
- Coverage 83.65% 72.14% -11.51%
===========================================
Files 318 318
Lines 28666 28735 +69
Branches 5464 5483 +19
===========================================
- Hits 23980 20731 -3249
- Misses 3168 6633 +3465
+ Partials 1518 1371 -147
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
8e0d3c6
to
a447834
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! 👍
@@ -32,7 +33,7 @@ class Add(Merge): | |||
def _merge_function(self, inputs): | |||
output = inputs[0] | |||
for i in range(1, len(inputs)): | |||
output = output + inputs[i] | |||
output = ops.add(output, inputs[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very strange that TF cannot +
sparse tensors...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed
3 x = tf.SparseTensor(indices=[[0, 0], [1, 2]], values=[2, 3], dense_shape=(3, 3))
----> 4 x + x
TypeError: unsupported operand type(s) for +: 'SparseTensor' and 'SparseTensor'
Only __div__
, __mul__
, __truediv__
are supported: https://www.tensorflow.org/api_docs/python/tf/sparse/SparseTensor
): | ||
import tensorflow as tf | ||
|
||
x = tf.SparseTensor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also test execution/correctness for the sparse/dense mixture case, i.e. x sparse and y dense or inversely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's exactly what this test is. x
and y
are sparse, z
is dense, I verify x + z
(sparse + dense), z + x
(dense + sparse) and x + y
(sparse + sparse).
I can add comments to clarify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reorganized the test to make it clear that the 3 combinations are tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
@@ -32,7 +33,7 @@ class Add(Merge): | |||
def _merge_function(self, inputs): | |||
output = inputs[0] | |||
for i in range(1, len(inputs)): | |||
output = output + inputs[i] | |||
output = ops.add(output, inputs[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed
3 x = tf.SparseTensor(indices=[[0, 0], [1, 2]], values=[2, 3], dense_shape=(3, 3))
----> 4 x + x
TypeError: unsupported operand type(s) for +: 'SparseTensor' and 'SparseTensor'
Only __div__
, __mul__
, __truediv__
are supported: https://www.tensorflow.org/api_docs/python/tf/sparse/SparseTensor
): | ||
import tensorflow as tf | ||
|
||
x = tf.SparseTensor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's exactly what this test is. x
and y
are sparse, z
is dense, I verify x + z
(sparse + dense), z + x
(dense + sparse) and x + y
(sparse + sparse).
I can add comments to clarify
a447834
to
2d246ad
Compare
Added `tf.SparseTensor` support for ops: - add - concatenate - maximum - minimum - multiply - subtract Added `tf.SparseTensor` support for merging layers: - Add - Average - Concatenate - Maximum - Minimum - Multiply - Subtract Note that the `Dot` merging layer will be addressed in a separate PR.
2d246ad
to
4698c0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Added
tf.SparseTensor
support for ops:Added
tf.SparseTensor
support for merging layers:Note that the
Dot
merging layer will be addressed in a separate PR.