Skip to content

[MRG+1] Feature or Bug? Whitened PCA does not inverse_transform properly and is even document that way #3107

Closed
wants to merge 4 commits into from
@eickenberg

The PCA.inverse_transform docstring even explicitly states that the inverse transform after whitening is inexact, because the necessary rescaling to fall back into the right space is not done. Is there a specific reason for this? One would think that the expected behaviour of an inverse_transform should be that it maps to the closest possible point in input space given the incurred information loss. Since with (full rank) PCA one can keep all the information, the inverse transform should be the true inverse.

Any opinions on this?

My main concern for changing this is that this behaviour is documented and thus possibly expected by some users.
Making the PCA object do a true inverse is as easy as adding 2 lines to the inverse_transform as visible in the diff.

@dengemann dengemann commented on the diff Apr 24, 2014
sklearn/decomposition/tests/test_pca.py
@@ -161,8 +161,7 @@ def test_pca_inverse():
pca.fit(X)
Y = pca.transform(X)
Y_inverse = pca.inverse_transform(Y)
- relative_max_delta = (np.abs(X - Y_inverse) / np.abs(X).mean()).max()
- assert_almost_equal(relative_max_delta, 0.11, decimal=2)
+ assert_almost_equal(X, Y_inverse, decimal=3)
@dengemann
dengemann added a note Apr 24, 2014

would this one fail on master?

@eickenberg
eickenberg added a note Apr 24, 2014

yes, I wrote it before coding the fix, and it failed

@eickenberg
eickenberg added a note Apr 24, 2014

Or rather "fix", because I am still not sure whether the current behaviour can be useful or not

@dengemann
dengemann added a note Apr 24, 2014

I'm surprised about the 3 decimals of precision

@GaelVaroquaux
scikit-learn member
@eickenberg
eickenberg added a note Apr 24, 2014

I just copied the same assertion statement from above, where it is tested with whiten=False. It should work at higher precision

@eickenberg
eickenberg added a note Apr 24, 2014

But wouldn't transforming back into the full space imply an un-whitening as well?

@GaelVaroquaux
scikit-learn member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@amueller
scikit-learn member

I find the current behavior unintuitive, to say the least, and agree with @eickenberg. Ugly proposed solution: introduce a parameter that changes the behavior, make the default False and add a warning that the default will change to True. Then a release later change the default, and one more release to remove the parameter. Hurray?

Or we could just break it ;)

@agramfort agramfort commented on the diff Apr 28, 2014
sklearn/decomposition/pca.py
@@ -403,13 +403,14 @@ def inverse_transform(self, X):
Returns
-------
X_original array-like, shape (n_samples, n_features)
-
- Notes
- -----
- If whitening is enabled, inverse_transform does not compute the
- exact inverse operation as transform.
@agramfort
scikit-learn member
agramfort added a note Apr 28, 2014

this comment dates back from 2011 so it seems this behavior as always been there

@GaelVaroquaux do you remember what was the rationale for this?

@GaelVaroquaux
scikit-learn member
@agramfort
scikit-learn member
agramfort added a note Apr 28, 2014
@eickenberg
eickenberg added a note Apr 28, 2014

I guess what makes everybody hesitant is that it was explicitly documented. Are there general notes on what inverse_transform should do? The rules I have been able to identify are something like inverse_transform(transform(point)) should be as close as possible to point for a metric appropriate to the situation and within a reasonable tradeoff of effort/closeness (in case this actually involves solving another optimization problem).

@agramfort
scikit-learn member
agramfort added a note Apr 28, 2014
@ogrisel
scikit-learn member
ogrisel added a note Jul 29, 2014

I think it's a bug as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arjoly arjoly added the Bug label May 11, 2014
@seanv507

Michael, Could you do the same fix for randomized PCA?

sean

@eickenberg

Yes, I can take care of changing this in all relevant modules if it is decided that this behaviour is wanted. In the mean time, I posted a fix to this on stack overflow which can be used without changing the scikit learn code: http://stackoverflow.com/questions/23254700/inversing-pca-transform-with-sklearn-with-whiten-true/23263923#23263923

@kastnerkyle
scikit-learn member

In my Incremental PCA PR (#3285) there is an introduction of a _BasePCA module. Though the other PCA modules have not yet been converted to use this as the base class, that is the direction we discussed in the PR comments. This is a great chance to a) unify common functions and b) provide deprecation support for the whole whiten/unwhiten transform issue in one place.

@eickenberg
@amueller
scikit-learn member

So does this wait for #3285? Or do we want to merge this first?

@seanv507

One last question/ request? currently whiten scales the components_ variable ( in order to rescale the transformed data) . should this be done? or should components_ vectors always have norm 1 (whether we do whiten or not)

@GaelVaroquaux
scikit-learn member
@eickenberg
@kastnerkyle
scikit-learn member
@amueller amueller added this to the 0.15.1 milestone Jul 18, 2014
@ogrisel
scikit-learn member
ogrisel commented Jul 29, 2014

One last question/ request? currently whiten scales the components_ variable ( in order to rescale the transformed data) . should this be done? or should components_ vectors always have norm 1 (whether we do whiten or not)

I think that the components should always have norm 1. We expect them to be a orthonormal basis.

+1 as well. This is another bug that should be tackled by this PR and properly documented. As this is quite an important semantic change I would be in favor of not including this fix in 0.15.1 but rather schedule that for 0.16.0 instead. Will change the milestone.

@ogrisel ogrisel closed this Jul 29, 2014
@ogrisel ogrisel reopened this Jul 29, 2014
@ogrisel
scikit-learn member
ogrisel commented Jul 29, 2014

oops, wrong comment button again. Sorry.

@ogrisel ogrisel modified the milestone: 0.15.1, 0.16 Jul 29, 2014
@coveralls

Coverage Status

Coverage increased (+0.22%) when pulling 5f5ef35 on eickenberg:pca_whiten_inverse_transform into f7e9527 on scikit-learn:master.

@eickenberg

Components are now always of unit length.

@coveralls

Coverage Status

Coverage decreased (-0.0%) when pulling 74e25ab on eickenberg:pca_whiten_inverse_transform into 25f72c4 on scikit-learn:master.

@ogrisel ogrisel commented on an outdated diff Aug 1, 2014
sklearn/decomposition/pca.py
@@ -356,8 +353,8 @@ def get_precision(self):
# Get precision using matrix inversion lemma
components_ = self.components_
exp_var = self.explained_variance_
- if self.whiten:
- components_ = components_ * np.sqrt(exp_var[:, np.newaxis])
+ # if self.whiten:
+ # components_ = components_ * np.sqrt(exp_var[:, np.newaxis])
@ogrisel
scikit-learn member
ogrisel added a note Aug 1, 2014

Please delete the commented out lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ogrisel
scikit-learn member
ogrisel commented Aug 1, 2014

This looks good to me, please add an entry in the whats new file. This change need to be clearly documented.

@coveralls

Coverage Status

Coverage decreased (-0.0%) when pulling b0ee936 on eickenberg:pca_whiten_inverse_transform into 25f72c4 on scikit-learn:master.

@coveralls

Coverage Status

Coverage increased (+0.0%) when pulling 7128600 on eickenberg:pca_whiten_inverse_transform into 25f72c4 on scikit-learn:master.

@eickenberg

Do I need to squash these?

@ogrisel
scikit-learn member
ogrisel commented Aug 1, 2014

Yes please. It helps to have a single commit per change that have a meaning on its own.

@eickenberg

Sorry for being slow: All of them into one, or, as I just pushed, into semantically coherent subunits? (I'm getting better at rebase -i :))

@ogrisel
scikit-learn member
ogrisel commented Aug 1, 2014

This is good, although you could prefix the commit messages with ENH FIX TST or DOC depending on their content.

@ogrisel
scikit-learn member
ogrisel commented Aug 1, 2014

you need to rebase on top of current master though. github says that you are based on an old master.

@eickenberg

I was rebased on a master commit of earlier today. Now it should be the latest one. Also added the descriptors to the commits. Thanks for the patience!

@coveralls

Coverage Status

Coverage decreased (-0.0%) when pulling 3d2be3b on eickenberg:pca_whiten_inverse_transform into 07560e4 on scikit-learn:master.

@ogrisel
scikit-learn member
ogrisel commented Aug 1, 2014

We usually don't put square brackets in the commits messages (just in the PR headers for some reason) but this is fine. +1 for merge on my side.

@ogrisel ogrisel changed the title from Feature or Bug? Whitened PCA does not inverse_transform properly and is even document that way to [MRG+1] Feature or Bug? Whitened PCA does not inverse_transform properly and is even document that way Aug 1, 2014
@coveralls

Coverage Status

Coverage decreased (-0.0%) when pulling 443312d on eickenberg:pca_whiten_inverse_transform into bf43d5b on scikit-learn:master.

@agramfort
scikit-learn member

Besides the rebase pb lgtm

Travis failure is unrelated

@ogrisel
scikit-learn member
ogrisel commented Aug 1, 2014

@eickenberg do you want me to cherry-pick your commits and push them to master or do you want to try to rebase one more time to get rid of my "FIX #3485: class_weight='auto' on SGDClassifier" spurious commit in the middle?

@eickenberg

Now there are only the 4 commits that I wrote. Unless I didn't mess up badly, this should be already rebased on the latest master and correct in content.

@MechCoder
scikit-learn member

The travis failure should go if this is rebased on master. there was a bug fix.

@coveralls

Coverage Status

Coverage decreased (-0.0%) when pulling 69604d5 on eickenberg:pca_whiten_inverse_transform into 22cafa6 on scikit-learn:master.

@kastnerkyle
scikit-learn member

This looks pretty good to me - I am a definite +1 . It will help for #3285 and future unification of the common code in PCA

@arjoly
scikit-learn member
arjoly commented Aug 6, 2014

Reviewers are happy. I am going to merge by rebase.

@arjoly
scikit-learn member
arjoly commented Aug 6, 2014

Merged by rebase.

@arjoly arjoly closed this Aug 6, 2014
@arjoly
scikit-learn member
arjoly commented Aug 6, 2014

Thanks @eickenberg. Is there an issue ticket for this?

@eickenberg
@eyaler
eyaler commented Mar 15, 2015

ZCA is a useful transformation defined as PCA + whitening + inverse rotation. this change breaks it.

@kastnerkyle
scikit-learn member
@eickenberg
@kastnerkyle
scikit-learn member
@eickenberg

Well, the way it was before was that the PCA.inverse_transform forgot to unwhiten if whiten=True.

Also, it scaled the lengths of the components, although orthonormality is expected.

Exploiting this, you could do pca = PCA(whiten=True); pca.inverse_transform(pca.transform(data)), and it basically gives you ZCA. With the fix, it gives you the signal back, which is what is expected.

To make a ZCA using PCA, you can do pca = PCA(whiten=False); t = pca.fit_transform(data); zca = pca.inverse_transform(data / np.sqrt(pca.explained_variance_)) give or take an array shape and a square root.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.