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

Whitening transform that supports mean subtraction #569

Closed
wants to merge 2 commits into from

Conversation

linzhiqiu
Copy link

I added a Whitening class that builds on the LinearTransformation class, but this one further supports pixel-wise zero centering using a mean vector computed offline, which is essential for ZCA whitening.

I added a Whitening module that supports mean subtraction using a mean vector computed offline.
@fmassa
Copy link
Member

fmassa commented Aug 13, 2018

Hi,

Thanks for the PR!

It looks like most of the code is taken from the LinearTransformation layer.
Can't we instead have a Normalize layer before the LinearTransformation in order to obtain the same results?

@linzhiqiu
Copy link
Author

linzhiqiu commented Aug 13, 2018

Thanks for the reply!

I think the Normalize layer is meant to normalize each channel separately - however, sometimes before we use ZCA we need a feature-wise normalization (i.e. pixel-wise). If you think this is better solved by a Normalize layer and a LinearTransformation layer, then we should probably modify the Normalize layer since right now it only says it supports channel-wise normalization.

@fmassa
Copy link
Member

fmassa commented Aug 14, 2018

Indeed, currently Normalize only supports passing around the same values for the channel normalization.
Still, I think it's simpler to have the user write a basic Normalilze that performs pointwise subtraction, than have an almost duplicate implementation of LinearTransformation.

@linzhiqiu
Copy link
Author

Maybe we could combine this mean subtraction feature into LinearTransformation and change it to a more suitable name? Unless pixel-wise mean subtraction is really uncommon for deep learning, otherwise they will need such a Transform which is currently not supported.

@fmassa
Copy link
Member

fmassa commented Sep 11, 2018

Sorry for the delay in replying.

I would be ok with adding the addition operation into LinearTransformation, but then it shouldn't be called LinearTransformation anymore, but AffineTransformation.

What about the following:

  • add an addition coefficient to LinearTransformation, and rename it to AffineTransformation
  • make LinearTransformation call AffineTransformation, and deprecate LinearTransformation

Could you do those changes?

This was referenced Mar 10, 2019
@fmassa
Copy link
Member

fmassa commented Mar 25, 2019

Closing in favor of #793

@fmassa fmassa closed this Mar 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants