Skip to content

Conversation

@frgfm
Copy link
Contributor

@frgfm frgfm commented May 6, 2020

This PR aims at tackling #2169 by:

  • adding a eps attribute to ̀FrozenBatchNorm2d`
  • renaming the n positional argument of FrozenBatchNorm2d constructor to num_features
  • adding a unittest to ensure default eps is zero for backward compatibility
  • adding a unittest to ensure valid computation when epsilon is higher than zero by comparison with BatchNorm2d forward in eval mode.

@frgfm
Copy link
Contributor Author

frgfm commented May 7, 2020

@fmassa I'm not sure where the error comes from, it seems like the tests timed out in test_ops at NMSTester. Do you think it could be due to this PR?

@frgfm
Copy link
Contributor Author

frgfm commented May 7, 2020

Also, I made two choices here that any reviewer might want to take a look at:

  • renaming the constructor argument n to get closer to BatchNorm2d interface (I didn't see many reasons for someone to use kwargs, but it might be worth mentioning it as a breaking changes if merged into the next release)

  • rearranging the forward implementation by switching reshape to view (I might have missed a use case where it's needed), and delaying this reshape/view to the last moment to reduce the LoC. I made tests to ensure the computation is the exact same as previously, and I also checked execution time on both CPU & GPU which were the same.

@zhangguanheng66
Copy link
Contributor

Also, I made two choices here that any reviewer might want to take a look at:

  • renaming the constructor argument n to get closer to BatchNorm2d interface (I didn't see many reasons for someone to use kwargs, but it might be worth mentioning it as a breaking changes if merged into the next release)
  • rearranging the forward implementation by switching reshape to view (I might have missed a use case where it's needed), and delaying this reshape/view to the last moment to reduce the LoC. I made tests to ensure the computation is the exact same as previously, and I also checked execution time on both CPU & GPU which were the same.

For the first option, I'm not sure if it introduces a BC breaking because it just re-defines the name of the argument.

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.

Thanks for the PR!

I have a couple of comments, could you look into addressing those?

@fmassa
Copy link
Member

fmassa commented May 11, 2020

Test failures seem related

@frgfm
Copy link
Contributor Author

frgfm commented May 11, 2020

Thanks, I fixed those. There is a timeout remaining for windows builds and an unrelated (I think) failure on travis!

@frgfm frgfm requested a review from fmassa May 11, 2020 14:00
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.

Thanks a lot!

@fmassa fmassa merged commit 7a2d061 into pytorch:master May 11, 2020
@frgfm frgfm deleted the frozenbn-eps branch May 11, 2020 14:50
fmassa pushed a commit to fmassa/vision-1 that referenced this pull request Jul 10, 2020
* feat: Added eps argument to FrozenBatchNorm2d

* test: Added unittest for eps addition in FrozenBatchNorm2d

See pytorch#2169

* fix: Reverted forward changes for JIT fuser

* fix: Added back n argument for backward-compatibility

* fix: Fixed FrozenBatchNorm2d forward

Added back eps

* feat: Specified deprecation warnings in FrozenBatchNorm2d

* test: Added unittest for deprecation warninig in FrozenBatchNorm2d

* style: Fixed lint

* style: Fixed block comment lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants