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

Sparse fill empty rows op #7126

Closed
wants to merge 46 commits into from

Conversation

codeislife99
Copy link
Contributor

@codeislife99 codeislife99 commented Dec 17, 2020

This PR is to support SparseFillEmptyRows Op (similar to https://www.tensorflow.org/api_docs/python/tf/sparse/fill_empty_rows) in relay. Frontend code for the TF Op will be added in a following PR since it combines multiple ops together)

@codeislife99 codeislife99 marked this pull request as draft December 17, 2020 22:54
@codeislife99 codeislife99 marked this pull request as ready for review December 21, 2020 11:18
@codeislife99
Copy link
Contributor Author

codeislife99 commented Dec 21, 2020

cc : @trevor-m @comaniac @zhiics PTAL

This was referenced Dec 22, 2020
@comaniac
Copy link
Contributor

@tkonolige @mbrookhart PTAL and approve or request changes explicitly. Thanks.

Copy link
Contributor

@tkonolige tkonolige left a comment

Choose a reason for hiding this comment

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

I've suggested some more small changes.

Given the discussion with @mbrookhart, I think you should use dynamic shape here.

include/tvm/topi/transform.h Outdated Show resolved Hide resolved
python/tvm/relay/op/transform.py Outdated Show resolved Hide resolved
src/relay/op/tensor/transform.cc Outdated Show resolved Hide resolved
src/relay/op/tensor/transform.cc Outdated Show resolved Hide resolved
src/relay/op/tensor/transform.cc Outdated Show resolved Hide resolved
src/relay/op/tensor/transform.cc Outdated Show resolved Hide resolved
src/relay/op/tensor/transform.cc Outdated Show resolved Hide resolved
@tkonolige
Copy link
Contributor

@comaniac Every time I select request changes on GitHub, it switches it "suggested changes". Maybe because I am not a committer?

@comaniac
Copy link
Contributor

@comaniac Every time I select request changes on GitHub, it switches it "suggested changes". Maybe because I am not a committer?

Yes that's the case, but we'll do our best to make sure all comments are addressed before merging.

Copy link
Contributor

@mbrookhart mbrookhart left a comment

Choose a reason for hiding this comment

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

I don't fully understand the point of these sparse op PRs, is there a particular model we're trying to target? It's hard for me to review without understanding the use case.

There are issues in this set of PRs that will prevent them from working on dynamically shaped input tensors. I don't think that's a blocker, but it could cause headaches down the road.

include/tvm/topi/transform.h Show resolved Hide resolved
Ubuntu and others added 11 commits December 30, 2020 11:17
Co-authored-by: Tristan Konolige <tristan.konolige@gmail.com>
Co-authored-by: Tristan Konolige <tristan.konolige@gmail.com>
Co-authored-by: Tristan Konolige <tristan.konolige@gmail.com>
Co-authored-by: Tristan Konolige <tristan.konolige@gmail.com>
Co-authored-by: Tristan Konolige <tristan.konolige@gmail.com>
@codeislife99
Copy link
Contributor Author

@tkonolige @mbrookhart This PR is ready for re-review as well whenever you are back from vacation.

@codeislife99
Copy link
Contributor Author

Hoping back onto this stream of work after a while, was involved with something else.
Since this PR had a lot of issues, it has been completely rewritten in #7442 and addresses all the existing comments in this PR. This Op has also been tested E2E with the customer model. Please review #7442.

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.

4 participants