Skip to content

Move LayerNorm to ATen; remove tracking_running_stats functionality #5983

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
Merged
soumith merged 2 commits into from
Mar 30, 2018
Merged

Move LayerNorm to ATen; remove tracking_running_stats functionality #5983

soumith merged 2 commits into from
Mar 30, 2018

Conversation

@ssnl
Copy link
Collaborator

@ssnl ssnl commented Mar 24, 2018

After discussing with @soumith , we decide to remove track_running_stats option from LayerNorm as it doesn't make much sense.

cc @soumith

@onnxbot-worker-1 onnxbot-worker-1 mentioned this pull request Mar 24, 2018
Closed
@ssnl ssnl force-pushed the ln branch 2 times, most recently from 4ab3226 to 3d7ff08 Compare March 24, 2018 20:56
@ssnl
Copy link
Collaborator Author

ssnl commented Mar 24, 2018

@pytorchbot retest this please

ssnl
ssnl commented Mar 24, 2018
View reviewed changes
aten/src/ATen/native/Normalization.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@ezyang ezyang requested a review from soumith March 26, 2018 18:10
zou3519
zou3519 reviewed Mar 29, 2018
View reviewed changes
aten/src/ATen/native/Normalization.cpp Outdated

This comment was marked as off-topic.

Sign in to view

@ssnl
Copy link
Collaborator Author

ssnl commented Mar 29, 2018

@zou3519 does the new commit look good to you? :)

zou3519
zou3519 approved these changes Mar 30, 2018
View reviewed changes
Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

Slightly concerned that we're not documenting torch.layer_norm and co's last argument (the cudnn_enabled) but it should be fine because users should interact with layer norm through nn.LayerNorm

image

@ssnl
Copy link
Collaborator Author

ssnl commented Mar 30, 2018

@zou3519 That's right. We have so many torch.* that are hidden from user.

@apaszke
Copy link
Contributor

apaszke commented Mar 30, 2018 •
edited
Loading

If we’re not planning on making a function part of our interface then can we please prefix it with an underscore?

@ssnl
Copy link
Collaborator Author

ssnl commented Mar 30, 2018

@apaszke Having the non-underscored version helps our ATen users. I think this is the reason why BN isn't called at::_batch_norm

@soumith soumith merged commit 48ad454 into pytorch:master Mar 30, 2018
@ssnl ssnl deleted the ln branch March 30, 2018 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zou3519 zou3519 zou3519 approved these changes

@soumith soumith Awaiting requested review from soumith

Assignees

No one assigned

Labels

open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ssnl @apaszke @zou3519 @ezyang @soumith