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.

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

Partially revert #519 due to performance regression & other issues #521

Merged
merged 1 commit into from
May 28, 2018
Merged

Partially revert #519 due to performance regression & other issues #521

merged 1 commit into from
May 28, 2018

Conversation

kohr-h
Copy link
Contributor

@kohr-h kohr-h commented May 28, 2018

Follow-up to #519, fixing the case of scalar mean and std in normalize.

@kohr-h
Copy link
Contributor Author

kohr-h commented May 28, 2018

The test failure is due to this not working:

>>> torch.Tensor(torch.tensor(0.5))
RuntimeError: slice() cannot be applied to a 0-dim tensor.

I can fix the code here by calling float() on the values not calling Tensor on tensors, but I think this is a pytorch bug.

@fmassa
Copy link
Member

fmassa commented May 28, 2018

I think this is getting quite messy.

Did you benchmark to see if performing a single sub followed by a div is faster than the 3 sub / div that we had before?
I think that in the other codepath we had vectorization that was enabled, and I'm not sure that's the case here.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented May 28, 2018

mean (sequence): Sequence of means for each channel.

According to the doc I have an impression that mean (same for std) is a sequence of scalars and not torch.tensors, e.g.

mean = [0.5, 0.5, 0.5]

or

mean = [0.5, ]

so in this case the previous code (ignoring the benchmark) works correctly

torch.Tensor(mean).view((n_channels, 1, 1))

@kohr-h
Copy link
Contributor Author

kohr-h commented May 28, 2018

Did you benchmark to see if performing a single sub followed by a div is faster than the 3 sub / div that we had before?
I think that in the other codepath we had vectorization that was enabled, and I'm not sure that's the case here.

I should have done that in the first place. The old version was actually faster 😒

According to the doc I have an impression that mean (same for std) is a sequence of scalars and not torch.tensors, e.g.

Correct if you see torch.tensor(0.5) not as a scalar, but you could also argue that anything on which you can run float() is a scalar (or complex() if necessary).

@kohr-h kohr-h changed the title Fix error in normalize with scalar mean/std Partially revert #519 due to performance regression & other issues May 28, 2018
@fmassa fmassa merged commit da67a1e into pytorch:master May 28, 2018
@kohr-h kohr-h deleted the fix_scalar_mean_std branch May 28, 2018 22:44
varunagrawal pushed a commit to varunagrawal/vision that referenced this pull request Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants