-
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
[ONNX] [Test] fix GRU modification and reduce tolerance for RNN tests #8923
Conversation
2c8fe0e
to
c32cb7a
Compare
Hey all, how did you check the required tolerance? Can you run it 1000 times to ensure these tolerances will pass if you have not already? |
It is a good question. Unfortunately there is only one test for operation (or its modification) to reduce testing time. Moreover numpy random is used with repeatable randomization. It means that it is not checked fully all cases for operation from which we could define tolerance thresholds. But it checks that operation works for one but orbitrary input for reasonable tolerance. For numerical calculations the tolerance of order of 1e-6 - 1e-5 seems reasonable. In my case it was high enough to hide problem in operation. |
I just want to make sure the tests are not super flaky. You can do this by running Lower than 1 / 1000 failures will seem sufficient. |
@AndrewZhaoLuo I have added seed for test reproduction and checked it 1000 times. |
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, though you'll need one of the code owners on the right to merge. @mbrookhart
…apache#8923) * fix high tolerance for RNN tests * random seed was added to GRU test reproduction Co-authored-by: Valery Chernov <valery.chernov@deelvin.com>
…apache#8923) * fix high tolerance for RNN tests * random seed was added to GRU test reproduction Co-authored-by: Valery Chernov <valery.chernov@deelvin.com>
During unification of GRU layer for frontends (#8781) the critical bag in GRU implementation on ONNX frontend side was observed. Inspite of unit test of GRU the bag was hidden by high tolerance (~1e-2) set for RNN tests. Moreover after bug fixing GRU unit test still requires high tolerance for correct pass. This fact was strange because local tests show very good coincidence with golden output from pytorch for any GRU modification (~1e-7).