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.

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

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

hertschuh
Copy link
Collaborator

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.

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -11.51% ⚠️

Comparison is base (0aa999b) 83.65% compared to head (4698c0e) 72.14%.

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     
Flag Coverage Δ
keras_core 72.09% <100.00%> (-11.46%) ⬇️
keras_core-jax 67.04% <48.10%> (-0.07%) ⬇️
keras_core-numpy ?
keras_core-tensorflow 66.86% <100.00%> (+0.07%) ⬆️
keras_core-torch ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
keras_core/backend/tensorflow/numpy.py 94.60% <100.00%> (+0.68%) ⬆️
keras_core/layers/merging/add.py 100.00% <100.00%> (ø)
keras_core/layers/merging/average.py 92.30% <100.00%> (+0.64%) ⬆️
keras_core/layers/merging/base_merge.py 41.08% <100.00%> (+2.37%) ⬆️
keras_core/layers/merging/multiply.py 92.30% <100.00%> (+0.64%) ⬆️
keras_core/layers/merging/subtract.py 81.25% <100.00%> (+1.25%) ⬆️
keras_core/ops/numpy.py 94.49% <100.00%> (+0.04%) ⬆️

... and 48 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@fchollet fchollet left a 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])
Copy link
Member

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...

Copy link
Collaborator Author

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(
Copy link
Member

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.

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@hertschuh hertschuh left a 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])
Copy link
Collaborator Author

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(
Copy link
Collaborator Author

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

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.
Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@fchollet fchollet merged commit a92593f into keras-team:main Sep 20, 2023
5 of 8 checks passed
@hertschuh hertschuh deleted the sparse_merging_layers branch September 20, 2023 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants