-
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
[Bug][relay][pytorch] Wrong implementation logic in Threshold operator #14805
Comments
Fixing this bug needs to master the complete code structure of Relay. I am not very familiar with this process. |
@jikechao Thank you for your finds. IMHO, fixing frontend bugs is also very important work. TVM also cares about the accuracy because if we have a good performance but bad accuracy then it makes no sense because the final result is incorrect. By fixing such bugs and adding new operators support, you make a great contribution to model coverage by TVM. I think it is an important work.
Also, it is my personal opinion, but usually I use official documentation, you can find there many useful articles. E.g.:
Another trick which I commonly use, I try to find in the source code or in the git history something similar to my task and understand which components I have to modify. For example, if I need to add new operator support, I would find another commit which adds some another operator to TVM and learn how it was done. |
@echuraev Thank you for your valuable suggestion. I will continue to learn and try to detect and fix frontend bugs. |
This issue has been fixed by the PR-14820 |
Relay has a wrong implementation logic about the
threshold
operator, It was used the same asrelu
. You can see Line 1334-1336 in file frontend/pytorch.pyThe implementation logic in PyTorch documentation is shown as follows:
This bug was detected by this test case:
Expected behavior
TVM gives same result as Pytorch
Actual behavior
TVM gave different result with Pytorch, It seems TVM produce the wrong result.
Triage
cc @Hzfengsy @echuraev @shingjan @yelite
The text was updated successfully, but these errors were encountered: