-
Notifications
You must be signed in to change notification settings - Fork 3.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
[Relay][Frontend] SparseTensorDenseMatMul support for Tensorflow #6685
Conversation
@FrozenGene , @siju-samuel , @kevinthesun : please help review. Thanks! |
Looks like Tensorflow version in CI is older, may be we need to bump it!!! |
cc @tkonolige |
|
||
# Create Numpy sparse Tensor(CSR) | ||
weight_sp = sparse.csr_matrix( | ||
(values_tensor, (rows, cols)), shape=tuple(dense_shape_tensor.tolist()) |
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.
If you swap rows and columns here you can avoid the sparse transpose below. This probably isn't much of a performance hit except for large matrices.
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 totally agree with your comment, but as this PR is just an initial PR, i feel lets keep the code as it is, it provides better readability in terms of steps involved, later on, once all features are merged, we can work together to optimize it 🙂
Please let me know, in case you think otherwise!
Gentle ping @tkonolige , @siju-samuel , Thanks! |
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.
Look pretty good to me. Just one change I think should be done.
What is the current review policy on TODO's in the code? Are they supposed to be tagged with a username?
I am already working on it. Will tag with my id here! |
@tkonolige : I think all your comments are addressed now! Please check and approve. Thanks! |
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.
Looks good to me. (I'm not an official reviewer, so you'll need someone else to approve it).
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.
Thanks @ANSHUMAN87 @tkonolige This PR is merged. |
…che#6685) * [Relay][Frontend] SparseTensorDenseMatMul support for Tensorflow * Lint error resolved * [1] Review comments handled * [2] Review comments handled
…che#6685) * [Relay][Frontend] SparseTensorDenseMatMul support for Tensorflow * Lint error resolved * [1] Review comments handled * [2] Review comments handled
…che#6685) * [Relay][Frontend] SparseTensorDenseMatMul support for Tensorflow * Lint error resolved * [1] Review comments handled * [2] Review comments handled
SparseTensorDenseMatMul support added for Tensorflow frontend.