-
Notifications
You must be signed in to change notification settings - Fork 7k
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
[BC-breaking] Change default eps value of FrozenBN #2933
Conversation
e21d669
to
ab66c93
Compare
ab66c93
to
7c8c5ae
Compare
Codecov Report
@@ Coverage Diff @@
## master #2933 +/- ##
=======================================
Coverage 73.41% 73.41%
=======================================
Files 99 99
Lines 8801 8801
Branches 1389 1389
=======================================
Hits 6461 6461
Misses 1915 1915
Partials 425 425
Continue to review full report at Codecov.
|
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.
Thanks for the PR!
It's great that the changes on accuracy don't seem to be affected in a significant way.
I suspect the results before the change are also the same values, or does it decrease mAP by 0.1?
I have one remark about the hack, I think it can be made such that we don't need to change the implementation in the op itself
Here are the baseline vs new comparisons: fasterrcnn_resnet50_fpn:
retinanet_resnet50_fpn:
maskrcnn_resnet50_fpn:
keypointrcnn_resnet50_fpn:
So overall I think we are OK. |
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.
Awesome, thanks a lot!
* Change default eps value of FrozenBN. * Update the unit-tests.` * Update the expected values. * Revert the expected value and use original eps=0 value for flaky tests. * Post init change of eps. * Styles.
* Change default eps value of FrozenBN. * Update the unit-tests.` * Update the expected values. * Revert the expected value and use original eps=0 value for flaky tests. * Post init change of eps. * Styles.
Fixes #2599
The good news is that changing the
FrozenBatchNorm2d
defaulteps
value from0.0
to1e-5
does not affect the accuracy of the detection models. You will see some tiny variations from the reported metrics but nothing concerning (could be the result of floating point errors). So I believe we can safely land the change without problems.Click here for the accuracy stats of fasterrcnn_resnet50_fpn
Click here for the accuracy stats of retinanet_resnet50_fpn
Click here for the accuracy stats of maskrcnn_resnet50_fpn
Click here for the accuracy stats of keypointrcnn_resnet50_fpn
The bad news is that the following detection unit-tests are brittle and changing the
eps
value causes them to break:I tried updating the expecting values to new ones but unfortunately there is too much variation across different devices and platforms. As observed at #2812, the flakiness is caused by floating point errors and the use of unstable sort. The problem is the result of initializing the model weights with random values and using random input in the unit-tests.
To fix the problem I introduce a VERY hacky workaround where I switch the default value to 0.0 for specific unit-tests. I really don't like this hack but I don't see how else we can keep the unit-tests on and while changing the
eps
. If anyone has another idea, I'm happy to give it a try.