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.

Sign up for GitHub

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

Jump to bottom

[topi][relay] add operation tan to TVM #4938

Merged
merged 8 commits into from
Mar 6, 2020
Merged

[topi][relay] add operation tan to TVM #4938

merged 8 commits into from
Mar 6, 2020

Conversation

notoraptor
Copy link
Contributor

Hi! This pull request adds operation tan to TVM. The main goal is to make it available in relay module.

@kevinthesun
Copy link
Contributor

Minor comments. Also can you rebase master?

@notoraptor
Copy link
Contributor Author

@kevinthesun Hi ! Thanks!

In order to fix failed test (precision error), I am trying to update the tan implementation in llvm target (by replacing sin/cos with a direct call to llvm's tan functions). But it currently generate other issues, so I am working on it.

I can rebase now, but I guess test will fail again.

@notoraptor
Copy link
Contributor Author

Rebased and updated to try to fix failing test.

@notoraptor
Copy link
Contributor Author

Hi @kevinthesun I rebased and added shape function for tan in latest commit.

I still have a mismatch error in tests/python/frontend/tensorflow/test_forward.py::test_forward_tan. It's hard to debug because I cannot reproduce it on my computer. But I am still working on it.

@notoraptor notoraptor force-pushed the relay-op-tan branch 3 times, most recently from 2623c57 to 138b3e7 Compare March 3, 2020 21:05
@notoraptor
Copy link
Contributor Author

@kevinthesun Tests passed ! I implemented tan in LLVM using sin/cos and I fixed CUDA implementation by using tanf instead of __tanf for float32.

kevinthesun
kevinthesun approved these changes Mar 6, 2020
View reviewed changes
Copy link
Contributor

@kevinthesun kevinthesun left a comment

Choose a reason for hiding this comment

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

LGTM

@kevinthesun kevinthesun merged commit d992468 into apache:master Mar 6, 2020
@kevinthesun
Copy link
Contributor

@notoraptor Thanks!

@tqchen
Copy link
Member

tqchen commented Mar 6, 2020

NOTE: the git commit message messed up today due to github issue https://discuss.tvm.ai/t/github-issue-the-commit-author-is-wrong-since-today/5880/15 we should refrain from merging others' PR until tomorrow

@kevinthesun
Copy link
Contributor

@tqchen Do we need to do something for this PR?

@tqchen
Copy link
Member

tqchen commented Mar 7, 2020 •
edited
Loading

See https://discuss.tvm.ai/t/response-to-github-author-attribution-bug/5909

@tqchen
Copy link
Member

tqchen commented Mar 9, 2020 •
edited
Loading

This PR is among one of the PRs affected by the github squash commit bug. We take every contribution serious in the TVM community. The community has decided to use revert/redo approach to amend the contributions as per #5015

@notoraptor
Copy link
Contributor Author

Hi @tqchen ! OK for that.

Anyway, if it causes some problems, then we can just let things as it is, as my main goal was just to make tan available in relay !

Thanks !

@tqchen tqchen mentioned this pull request Mar 9, 2020
Closed
5 tasks
@tqchen
Copy link
Member

tqchen commented Mar 9, 2020

cc @kevinthesun please send the reverting PR, then @notoraptor can send the patch

kevinthesun added a commit to kevinthesun/tvm that referenced this pull request Mar 9, 2020
@kevinthesun
Revert "[topi][relay] add operation tan to TVM (apache#4938)"
fedc0c6 
This reverts commit d992468.
@kevinthesun kevinthesun mentioned this pull request Mar 9, 2020
Merged
masahi pushed a commit that referenced this pull request Mar 10, 2020
@kevinthesun
Revert "[topi][relay] add operation tan to TVM (#4938)" (#5017)
c0bc188 
This reverts commit d992468.
@notoraptor notoraptor mentioned this pull request Mar 10, 2020
Merged
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Apr 16, 2020
@kevinthesun
[topi][relay] add operation tan to TVM (apache#4938)
ef6edfe 
* Add relay operation relay.op.tan.

* Update tan implementation in TVM.

* Update tests.

* Add shape function for tan.

* Add missing main test to python/frontend/tensorflow/test_forward.

* Revert, back to sin/cos.

* Revert "Revert, back to sin/cos."

This reverts commit 4da5b50.

* Fix implementation of tan in cuda. Do not support tan for float16.

Simplify topi/tests/python/test_topi_math. Add testing for tan with float32 and float64.

Try again to implement tan as sin/cos in llvm.
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Apr 16, 2020
@kevinthesun
Revert "[topi][relay] add operation tan to TVM (apache#4938)" (apache…
105a366 
…#5017)

This reverts commit d992468.
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Apr 17, 2020
@kevinthesun @zhiics
[topi][relay] add operation tan to TVM (apache#4938)
7ffaa99 
* Add relay operation relay.op.tan.

* Update tan implementation in TVM.

* Update tests.

* Add shape function for tan.

* Add missing main test to python/frontend/tensorflow/test_forward.

* Revert, back to sin/cos.

* Revert "Revert, back to sin/cos."

This reverts commit 4da5b50.

* Fix implementation of tan in cuda. Do not support tan for float16.

Simplify topi/tests/python/test_topi_math. Add testing for tan with float32 and float64.

Try again to implement tan as sin/cos in llvm.
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Apr 17, 2020
@kevinthesun @zhiics
Revert "[topi][relay] add operation tan to TVM (apache#4938)" (apache…
87c2ec1 
…#5017)

This reverts commit d992468.
@ZihengJiang ZihengJiang mentioned this pull request Sep 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinthesun kevinthesun kevinthesun approved these changes

@Huyuwei Huyuwei Awaiting requested review from Huyuwei

@Laurawly Laurawly Awaiting requested review from Laurawly

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@notoraptor @kevinthesun @tqchen