[MRG + 1] DOC adding note for bessel correction #7843

dalmia · 2016-11-08T20:05:11Z

Reference Issue

Fixes #7699

What does this implement/fix? Explain your changes.

Added a note to the docstring explaining the reason for not including bessel correction.

amueller · 2016-11-09T22:03:43Z

check the travis error...

dalmia · 2016-11-10T03:53:59Z

@amueller Please have a look now.

agramfort · 2016-11-10T21:23:05Z

sklearn/decomposition/pca.py

@@ -120,6 +120,9 @@ class PCA(_BasePCA):
    Notice that this class does not support sparse input. See
    :class:`TruncatedSVD` for an alternative with sparse data.

+    No bessel correction is applied to avoid the expense of increasing the mean
+    square error.


I find this message cryptic without reference or explanation

I'll add the explanation as discussed in the issue thread.

dalmia · 2016-11-11T14:26:45Z

@agramfort Do you feel this explains the case?

amueller · 2016-11-22T19:25:22Z

sklearn/decomposition/pca.py

@@ -120,6 +120,10 @@ class PCA(_BasePCA):
    Notice that this class does not support sparse input. See
    :class:`TruncatedSVD` for an alternative with sparse data.

+    Although Bessel correction decreases the bias of the estimator, it also


This is still pretty confusing and apropos of nothing. I'm kinda tempted not to add the note if we can't come up with a good but not too-long explanation here.

"PCA uses the maximum likelihood estimate of the eigenvalues, which does not include the Bessel correction, though in practice this should rarely make a difference." ?

Without a reference this indeed seems odd... +1 for your quoted wording. It feels clearer to me.

Sorry for being late. Could we add the link to the issue there as the discussion there makes it pretty clear? I feel we should include why we aren't including the bessel correction so as to avoid the expense of mean square error. Please let me know what you think.

dalmia · 2016-11-29T20:42:44Z

I would be really grateful if someone could make a rebuild. Thanks.

raghavrv · 2016-11-29T20:48:59Z

Could you rebase and forcepush. Otherwise LGTM

raghavrv · 2016-11-29T20:49:34Z

Or even update master and simply push. (Unless you are already updated with master that should trigger a rebuild)

amueller · 2016-11-29T21:53:39Z

@agramfort what do you think? It's fine for me, I'm not entirely sure if it's a helpful addition

dalmia · 2016-12-08T03:04:48Z

Do we have an opinion on his yet?

jnothman

otherwise this is fine by me

jnothman · 2016-12-14T12:18:54Z

sklearn/decomposition/pca.py

@@ -120,6 +120,10 @@ class PCA(_BasePCA):
    Notice that this class does not support sparse input. See
    :class:`TruncatedSVD` for an alternative with sparse data.

+    PCA uses the maximum likelihood estimate of the eigenvalues, which does not


I think this is fairly minor, so create a Notes section below where this can be included.

jnothman · 2016-12-14T12:19:02Z

sklearn/decomposition/pca.py

@@ -120,6 +120,10 @@ class PCA(_BasePCA):
    Notice that this class does not support sparse input. See
    :class:`TruncatedSVD` for an alternative with sparse data.

+    PCA uses the maximum likelihood estimate of the eigenvalues, which does not
+    include the Bessel correction, though in practice this should rarely make a


line length should be < 80.

Checked. The line length is below 80. Making the above change.

jnothman · 2016-12-15T21:12:15Z

LGTM

* fix pca explained_variance_ * fix fit_transform * fix test_whitening * fix IncrementalPCA * uncomment the test * improve test * make CI green * revert #7843 and add what's new * fix what's new

…t-learn#9105) * fix pca explained_variance_ * fix fit_transform * fix test_whitening * fix IncrementalPCA * uncomment the test * improve test * make CI green * revert scikit-learn#7843 and add what's new * fix what's new

agramfort requested changes Nov 10, 2016

View reviewed changes

raghavrv added the Documentation label Nov 14, 2016

raghavrv added this to the 0.19 milestone Nov 14, 2016

dalmia changed the title ~~[WIP] DOC adding note for bessel correction~~ [MRG] DOC adding note for bessel correction Nov 16, 2016

amueller reviewed Nov 22, 2016

View reviewed changes

raghavrv approved these changes Nov 29, 2016

View reviewed changes

raghavrv changed the title ~~[MRG] DOC adding note for bessel correction~~ [MRG + 1] DOC adding note for bessel correction Nov 29, 2016

dalmia closed this Nov 29, 2016

dalmia force-pushed the 7699 branch from 5f7da60 to 41e1b8f Compare November 29, 2016 21:08

FIX: adding note for bessel correction

e20910b

dalmia reopened this Nov 29, 2016

amueller mentioned this pull request Dec 5, 2016

No Bessel correction in PCA #7699

Closed

jnothman reviewed Dec 14, 2016

View reviewed changes

DOC: added Notes section

1243606

jnothman merged commit 1144e3b into scikit-learn:master Dec 15, 2016

dalmia deleted the 7699 branch December 15, 2016 22:10

sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

DOC adding note regarding bessel correction in PCA (scikit-learn#7843)

edd17d2

Przemo10 mentioned this pull request Mar 17, 2017

update fork (#1) #8606

Closed

Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

DOC adding note regarding bessel correction in PCA (scikit-learn#7843)

17b920e

qinhanmin2014 mentioned this pull request Jun 21, 2017

[MRG+2] Incorrect implementation of explained_variance_ in PCA #9105

Merged

NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

DOC adding note regarding bessel correction in PCA (scikit-learn#7843)

97b6d44

paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

DOC adding note regarding bessel correction in PCA (scikit-learn#7843)

44f3957

qinhanmin2014 mentioned this pull request Nov 15, 2017

PCA implementation does not match Tipping and Bishop #10137

Closed

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

DOC adding note regarding bessel correction in PCA (scikit-learn#7843)

91511f0

Uh oh!

[MRG + 1] DOC adding note for bessel correction #7843

[MRG + 1] DOC adding note for bessel correction #7843

Uh oh!

Conversation

dalmia commented Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issue

What does this implement/fix? Explain your changes.

Uh oh!

amueller commented Nov 9, 2016

Uh oh!

dalmia commented Nov 10, 2016

Uh oh!

agramfort Nov 10, 2016

Choose a reason for hiding this comment

Uh oh!

dalmia Nov 11, 2016

Choose a reason for hiding this comment

Uh oh!

dalmia commented Nov 11, 2016

Uh oh!

amueller Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

raghavrv Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

dalmia Nov 26, 2016

Choose a reason for hiding this comment

Uh oh!

dalmia commented Nov 29, 2016

Uh oh!

raghavrv commented Nov 29, 2016

Uh oh!

raghavrv commented Nov 29, 2016

Uh oh!

amueller commented Nov 29, 2016

Uh oh!

dalmia commented Dec 8, 2016

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

jnothman Dec 14, 2016

Choose a reason for hiding this comment

Uh oh!

jnothman Dec 14, 2016

Choose a reason for hiding this comment

Uh oh!

dalmia Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Dec 15, 2016

Uh oh!

Uh oh!

dalmia commented Nov 8, 2016 •

edited

Loading

dalmia Dec 15, 2016 •

edited

Loading