Skip to content

Clarify track_running_stats docs; Make SyncBatchNorm track_running_stats behavior consistent #44445

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
Closed
ssnl wants to merge 1 commit into from
Closed

Clarify track_running_stats docs; Make SyncBatchNorm track_running_stats behavior consistent #44445

ssnl wants to merge 1 commit into from

Conversation

@ssnl
Copy link
Collaborator

@ssnl ssnl commented Sep 10, 2020

context: #38084

Fixes #{issue number}

@ssnl ssnl requested a review from apaszke as a code owner September 10, 2020 01:10
@mrshenli mrshenli added the module: docs Related to our documentation, both in docs/ and docblocks label Sep 10, 2020
mrshenli
mrshenli reviewed Sep 10, 2020
View reviewed changes
torch/nn/modules/batchnorm.py
exponential_average_factor = self.momentum

need_sync = self.training or not self.track_running_stats
r"""
Copy link
Contributor

@mrshenli mrshenli Sep 10, 2020

Choose a reason for hiding this comment

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

Any reason for using rstring instead of comments for the following two doc blocks?

Copy link
Collaborator Author

@ssnl ssnl Sep 10, 2020

Choose a reason for hiding this comment

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

hmm not really. They were strings from _BatchNorm so I kept them this way when copying.

@mrshenli mrshenli added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Sep 10, 2020
@dr-ci
Copy link

dr-ci bot commented Sep 10, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 2f20f1d (more details on the Dr. CI page):

  • 3/3 failures possibly* introduced in this PR
    • 3/3 non-CircleCI failure(s)

Extra GitHub checks: 1 failed

ci.pytorch.org: 1 failed

codecov.io: 1 failed

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 3 times.

@codecov
Copy link

codecov bot commented Sep 10, 2020 •
edited
Loading

Codecov Report

Merging #44445 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #44445      +/-   ##
==========================================
- Coverage   67.99%   67.98%   -0.01%     
==========================================
  Files         382      382              
  Lines       49385    49391       +6     
==========================================
  Hits        33579    33579              
- Misses      15806    15812       +6
Impacted Files Coverage Δ
torch/nn/modules/batchnorm.py 73.97% <0.00%> (-3.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e028ad0...2f20f1d. Read the comment docs.

facebook-github-bot
facebook-github-bot reviewed Sep 10, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@mrshenli has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ssnl ssnl mentioned this pull request Sep 10, 2020
Open
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 11, 2020

@mrshenli merged this pull request in 09892de.

@ssnl ssnl deleted the SsnL-patch-2 branch September 14, 2020 17:10
xuzhao9 pushed a commit that referenced this pull request Sep 18, 2020
@ssnl @xuzhao9
Clarify track_running_stats docs; Make SyncBatchNorm track_running_st…
cd4a6ab 
…ats behavior consistent (#44445)

Summary:
context: #38084

Fixes #{issue number}

Pull Request resolved: #44445

Reviewed By: colesbury

Differential Revision: D23634216

Pulled By: mrshenli

fbshipit-source-id: d1242c694dec0e7794651f8031327625eb9989ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@mrshenli mrshenli mrshenli approved these changes

@apaszke apaszke Awaiting requested review from apaszke

Assignees

No one assigned

Labels

Merged module: docs Related to our documentation, both in docs/ and docblocks open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ssnl @facebook-github-bot @mrshenli @pytorchbot @mruberry