Skip to content

Conversation

@szagoruyko
Copy link
Contributor

works without eps with pretrained ResNet, but WRN and ResNeXt explode, thus this fix. Not sure about BC, trained ResNet models will slightly change.

to avoid activation explode
@szagoruyko
Copy link
Contributor Author

looks like tests are failing due to numerical diff w existing models

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

The PR generally looks good to me, thanks Sergey!

We would need 2 things before we can merge this:

  • verify the impact on the accuracy of the pre-trained models, for Faster R-CNN, Mask R-CNN and Keypoint R-CNN
  • if the impact on the accuracy is negligible, I push the new expected file for keypoint rcnn (run the test with --accept)
  • if the accuracy is affected in a non-trivial manner, set the default of the eps to zero, so that users can specify their own lambda that passes a different default value

Thoughts?

@frgfm
Copy link
Contributor

frgfm commented May 6, 2020

Hello there,

locally I made a patch with eps=0 argument to avoid breaking changes and figure the changes on the default value and its impact on model training performances could be investigated in a different issue/PR.

@szagoruyko happy to help if you need some assistance!

@szagoruyko
Copy link
Contributor Author

Hi @frgfm sorry I don't have time to work on this, please take over!

@szagoruyko szagoruyko closed this May 6, 2020
@frgfm
Copy link
Contributor

frgfm commented May 7, 2020

For the sake of documentation, if someone ends up here looking for the follow-up PR, here it is #2190 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants