Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Add option for
BatchNorm
to handle batches of size one #5530Add option for
BatchNorm
to handle batches of size one #5530Changes from 2 commits
df194c5
5398b65
d227042
24bbb6f
8cd8690
7ab163d
2709908
c1a86d9
87e50cf
39c250d
f86ef20
611edb0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm just saw this. Sorry I'm not clear on why this is the desired behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this
True
by default?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally feel making it False by default might be better, as its a bit of a corner case and doesn't match the pytorch behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not sure about this while looking at https://github.com/pytorch/pytorch/blob/master/torch/nn/modules/batchnorm.py#L53. I think
running_mean
is onlyNone
in case oftrack_running_stats=False
, in which case we should also error out.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hmm, yes, you're right, the problem only happens if
track_running_stats
isFalse
.In our current implementation, if
track_running_stats=False
we do not raise an exception in training mode. What would be the logic for us to do so in this specific case?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests in
test/nn/norm/test_batch_norm.py
all pass with this implementation, removing this will throw an error in the single combinationtrack_running_stats=False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is impossible to support
track_running_stats=False
withbatch_size=1
. The current implementation does simply not do any normalization which is probably not desired. I would simply error out in this case TBH.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah I guess that makes the most sense to me. Its a bit ambiguous but I haven't got a concrete use case for supporting this either so have updated based on your suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this really be
None
? I assume they will be initialized by PyTorch.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes: https://github.com/pytorch/pytorch/blob/9c036aa112b0a8fd9afb824d1fda058e2b66ba1d/torch/nn/modules/batchnorm.py#L68
Its a bit of an odd case, but this combined with: https://github.com/pytorch/pytorch/blob/9c036aa112b0a8fd9afb824d1fda058e2b66ba1d/torch/nn/modules/batchnorm.py#L175 will cause errors
As a side note I suspect this line should be:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems would cause a device mismatch if the module is already on CUDA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I guess the important thing is probably to match the device of
x
. But to simplify I just added them to init