-
Notifications
You must be signed in to change notification settings - Fork 19.5k
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
Added SparseTensor check on convert_to_np_if_not_ragged. #20151
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20151 +/- ##
=======================================
Coverage 79.35% 79.35%
=======================================
Files 501 501
Lines 47311 47313 +2
Branches 8689 8690 +1
=======================================
+ Hits 37542 37544 +2
Misses 8014 8014
Partials 1755 1755
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -905,6 +905,8 @@ def is_tpu_strat(k): | |||
def convert_to_np_if_not_ragged(x): | |||
if isinstance(x, tf.RaggedTensor): | |||
return x | |||
elif isinstance(x, tf.SparseTensor): | |||
return x |
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.
Would it make sense to instead return a scipy sparse matrix?
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.
In my case, this was a test to see if my layer would behave properly inside a model. So I am not using the output directly. The output will be sent to another layer.
I see no need for scipy in my use case, but it all depends on what people expect from the Model.predict() function.
What would have been really useful is that the error would say that, when using predict(), the output of the model needs to be a RaggedTensor, SparceTensor or something with a "numpy" attribute. I've spent quite some time trying to find out what I was doing wrong. However, I have no clue on where to place that check.
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.
I think it makes sense to return a TF SparseTensor in this case. The underlying issue is basically a TF limitation -- .numpy()
is a standard API and it's not normal that it isn't available on a SparseTensor
.
Can you add a unit test for this change?
@fchollet However, I don't understand the fail in the code format test. Can you help me make sense of it and understand how to fix it? |
The problem occurs when Model.predict() returns a sparse matrix.
Example code:
Proposed solution:
check for sparce tensors in keras/src/backend/tensorflow/trainer.py,