Skip to content

Conversation

@surgan12
Copy link
Contributor

enhancement #680.

@surgan12
Copy link
Contributor Author

@fmassa would love to hear from you on this !

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for the PR!

I believe there is a potential simplifications in the code.

Let me know what you think.


else:
# This is faster than using broadcasting, don't change without benchmarking
for t, m, s in zip(tensor, mean, std):
Copy link
Member

Choose a reason for hiding this comment

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

I believe this can now be replaced with something simpler and as efficient as before

mean = torch.tensor(mean, dtype=torch.float32)
std = torch.tensor(std, dtype=torch.float32)
return (tensor - mean[:, None, None]) / std[:, None, None]

This is out-of-place, but can be made in-place as well if needed

Just to double check, can you do some benchmarkings and report back if it's indeed now giving the same runtime speed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fmassa i ran some tests and found that this approach was smewhat faster .

t.sub_(m).div_(s)
return tensor
if not inplace:
tensor_clone = tensor.clone()
Copy link
Member

Choose a reason for hiding this comment

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

Can't you just do

if not inplace:
    tensor = tensor.clone()
# everything as before

?

t.sub_(m).div_(s)
return tensor
if not inplace:
tensor = tensor.clone()
Copy link
Member

Choose a reason for hiding this comment

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

Can we just have a single clone here, to avoid duplicated code?
like

if not inplace:
    tensor = tensor.clone()

mean = ...

@surgan12
Copy link
Contributor Author

@fmassa how does it look now ?

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

2 participants