Skip to content

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

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
Merged
kevinthesun merged 8 commits into from
Mar 6, 2020
Merged

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

kevinthesun merged 8 commits into from
Mar 6, 2020

Conversation

@notoraptor
Copy link
Contributor

@notoraptor notoraptor commented Feb 25, 2020

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

kevinthesun
kevinthesun reviewed Feb 27, 2020
View reviewed changes
@kevinthesun
Copy link
Contributor

kevinthesun commented Feb 27, 2020

Minor comments. Also can you rebase master?

@notoraptor
Copy link
Contributor Author

notoraptor commented Feb 27, 2020

@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

notoraptor commented Feb 27, 2020

Rebased and updated to try to fix failing test.

@notoraptor
Copy link
Contributor Author

notoraptor commented Feb 28, 2020

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
Copy link
Contributor Author

notoraptor commented Mar 4, 2020

@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

kevinthesun commented Mar 6, 2020

@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

kevinthesun commented Mar 6, 2020

@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

notoraptor commented Mar 9, 2020

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