Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Change GroupNormalization default groups to 32. #2241

Merged
merged 6 commits into from
Nov 18, 2020
Merged

Change GroupNormalization default groups to 32. #2241

merged 6 commits into from
Nov 18, 2020

Conversation

aaronmondal
Copy link
Contributor

@aaronmondal aaronmondal commented Nov 16, 2020 •
edited
Loading

This changes the default groups parameter to 32, as recommended by the authors of Group Normalization.

Fixes #2240

  • This PR addresses an already submitted issue for TensorFlow Addons

@boring-cyborg boring-cyborg bot added the layers label Nov 16, 2020
@google-cla google-cla bot added the cla: yes label Nov 16, 2020
@bot-of-gabrieldemarmiesse
Copy link

@Smokrow

You are owner of some files modified in this pull request.
Would you kindly review the changes whenever you have the time to?
Thank you very much.

@aaronmondal aaronmondal reopened this Nov 16, 2020
@aaronmondal
Copy link
Contributor Author

To clarify: test_regularizations was modified to accommodate the default of 32. A group size of 32 was already tested in test_initializer

Harsh188
Harsh188 reviewed Nov 16, 2020
View reviewed changes
Copy link
Contributor

@Harsh188 Harsh188 left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for contributing. Could you add to the documentation by specifying the default number of groups used?

@aaronmondal
Copy link
Contributor Author

Edited the Arguments section and added the source paper as well in the same way it was done for tf.nn.swish. Linking to papers was done differently for e.g. tf.nn.silu, but I think it is better to display that link explicitly so users immediately see the link target.

@AakashKumarNain
Copy link
Member

LGTM! Thanks @aaronmondal and @Harsh188

@AakashKumarNain AakashKumarNain merged commit b131407 into tensorflow:master Nov 18, 2020
@aaronmondal aaronmondal deleted the aaronmondal-groupnorm-to-32 branch November 18, 2020 23:38
@aaronmondal
Copy link
Contributor Author

Thank you @Harsh188 and @AakashKumarNain for your quick review and approval :)

jrruijli pushed a commit to jrruijli/addons that referenced this pull request Dec 23, 2020
@aaronmondal
Change GroupNormalization default groups to 32. (tensorflow#2241)
a2672c3 
* Change GroupNormalization default groups to 32.

* Explicitly set groups for GroupNormalization test to 2.

* Update documentation.

* Fix whitespace.

* Remove unnecessary reference.

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

@AakashKumarNain AakashKumarNain AakashKumarNain approved these changes

@Harsh188 Harsh188 Harsh188 approved these changes

Assignees
No one assigned
Labels
cla: yes layers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set GroupNormalization default groups parameter to 32.
4 participants
@aaronmondal @bot-of-gabrieldemarmiesse @AakashKumarNain @Harsh188