[C++ API] Document BatchNorm and update default behavior#11484
[C++ API] Document BatchNorm and update default behavior#11484goldsborough wants to merge 3 commits intopytorch:masterfrom
Conversation
|
Now do we want the |
|
|
|
@pytorchbot retest this please |
|
I don't have a good opinion about the momentum parameter, I might defer this to some of the vision folks? I think it might be a discussion for a future PR in any case. |
|
@prigoyal said that momentum is a universally well understood term so renaming it here probably doesn't make too much sense. |
ezyang
left a comment
There was a problem hiding this comment.
Not sure why you need code owner review on this one.
soumith
left a comment
There was a problem hiding this comment.
Do not diverge the PyTorch and C++ APIs. If we have BatchNorm momentum at the python level, for better or worse we stick to it.
API divergence is a much worse experience than academic correctness.
This reverts commit 27de526152e2ee184a632d4826386f0e7f3f19d0.
27de526 to
5209943
Compare
Addressed in latest revert commit
facebook-github-bot
left a comment
There was a problem hiding this comment.
goldsborough has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
goldsborough has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
* master: (165 commits) Aibench for asr decoder Explicitly set locale on docs build. (pytorch#11595) Documentation for debugging JIT Fused weightnorm for ATen (pytorch#10842) Move Type, Tensor, TensorMethods to core. Add reminder % to the jit Fix reloading modules back into python (pytorch#11552) Add trigonometry functions to docs/source/onnx.rst Add EndToEndHybridModel CUDA tests (pytorch#11544) minor formatting error log (pytorch#11528) Warn that export+import module always load onto the CPU (pytorch#11485) caffe2::StorageImpl use at::DataPtr (pytorch#11282) Sync all libnccl soversions, not just libnccl.so.1 (pytorch#11575) Document BatchNorm and update default behavior (pytorch#11484) Typo fix in randomness.rst (pytorch#11571) Move some bmm/baddbmm to ATen (pytorch#11292) Make c10d test work on CPU only build (pytorch#11567) Clean up some C++ cruftiness in the script lexer. Allow setting deletion constant Make C10d support CPU only build (pytorch#11513) ...
Summary:
This PR:
1. Documents `BatchNorm`,
2. Makes a number of API changes after reconsidering some quirks:
1. The default value for the `stateful` parameter used to be `false`, but the most common usage of `BatchNorm` out of the wild is certainly stateful, and the default in Python is also statefulness. So we change the default to stateful.
2. The `pure_forward` function used to use the internal running mean and variance variables instead of the ones supplied to that function call when `stateful` was true, which certainly seems odd. When you call `pure_forward` you would certainly expect the values you pass explicitly to be used. This is now fixed.
3. Adds tests for `BatchNorm`, finally.
ebetica apaszke ezyang
Pull Request resolved: pytorch#11484
Reviewed By: pjh5
Differential Revision: D9779618
Pulled By: goldsborough
fbshipit-source-id: 59ba760e085c01454b75644b24b22317b688e459
This PR:
BatchNorm,statefulparameter used to befalse, but the most common usage ofBatchNormout of the wild is certainly stateful, and the default in Python is also statefulness. So we change the default to stateful.pure_forwardfunction used to use the internal running mean and variance variables instead of the ones supplied to that function call whenstatefulwas true, which certainly seems odd. When you callpure_forwardyou would certainly expect the values you pass explicitly to be used. This is now fixed.BatchNorm, finally.@ebetica @apaszke @ezyang