[MRG+1] Correcting length of explained_variance_ratio_, eigen solver, final PR by JPFrancoia · Pull Request #7632 · scikit-learn/scikit-learn

JPFrancoia · 2016-10-10T21:23:52Z

Reference Issue

What does this implement/fix? Explain your changes.

Attribute explained_variance_ratio_ from LinearDiscriminantAnalysis class will be of length n_components (eigen solver).

Any other comments?

This PR follows PR 7616. I mixed up my git history, so it was easier to open a new PR.

LinearDiscriminantAnalysis class will be of length n_components, if provided. If not provided, will have a maximum length of n_classes. The attribute will have the same length, whatever the solver (SVD, Eigen).

…variance_ratio.

jnothman · 2016-10-10T23:12:06Z

doc/whats_new.rst

   - Access to public attributes ``.X_`` and ``.y_`` has been deprecated in
     :class:`isotonic.IsotonicRegression`. By `Jonathan Arfa`_.

+   - Length of `explained_variance_ratio` of :clas:`discriminant_analysis.LinearDiscriminantAnalysis`


Still cannot be under 0.18.0

jnothman · 2016-10-10T23:12:09Z

doc/whats_new.rst

-      :class:`discriminant_analysis.LinearDiscriminantAnalysis` now returns
-      correct results. By `JPFrancoia`_
+      :clas:`discriminant_analysis.LinearDiscriminantAnalysis` now returns correct results.
+      Attribute `explained_variance_ratio` calculated with SVD and Eigen solver are now of


Still cannot be under 0.18.0

JPFrancoia · 2016-10-11T06:37:05Z

Ok. Under what should it be then ? We are almost there (and no more test failures).

jnothman · 2016-10-12T12:34:31Z

Ok. Under what should it be then ? We are almost there (and no more test failures).

@amueller, should we create a 0.18.1 heading in what's new in master?

jnothman · 2016-10-12T12:48:15Z

There's now a 0.18.1 section

0.18.1. I created the sub-section "API changes summary" under 0.18.1.

JPFrancoia · 2016-10-12T13:06:18Z

Ok, I put my changes under 0.18.1. I created the sub-section API changes summary under 0.18.1 too.

jnothman · 2016-10-12T22:39:09Z

doc/whats_new.rst

You've repeatedly use :clas:. Should be :class:

Arf. Sorry to waste your time with a typo.

jnothman · 2016-10-13T06:36:07Z

doc/whats_new.rst

     `#6497 <https://github.com/scikit-learn/scikit-learn/pull/6497>`_
     by `Sebastian Säger`_

+   - Attribute ``explained_variance_ratio`` of :class:`discriminant_analysis.LinearDiscriminantAnalysis`


I can waste both of our time a little further by asking that you make the lines somewhat shorter (around 80 chars max) and insert a link to the issue/PR which seems to have been adopted here... Sorry.

No problem. Done.

jnothman · 2016-10-13T06:36:33Z

Otherwise LGTM

amueller · 2016-10-13T19:31:44Z

sklearn/discriminant_analysis.py


+        # Get maximum length for explained_variance_ratio_
+        if self.n_components is None:
+            max_length_exp = len(self.classes_)


shouldn't it be len(self.classes_) - 1?

I don't think so. If n_components is not provided, then explained_variance_ratio_ will have a length of maximum len(self.classes). So [:max_length_exp]

Well, that's a bug, the last one will always be zero.

amueller

explained_variance_ratio_ definitely needs to be at most of length n_classes - 1

JPFrancoia · 2016-10-15T08:24:10Z

Please explain why you think it should be len(self.classes_) - 1, I don't understand what you mean.

If I introduce this change, the tests fail:

nosetests -v sklearn.tests.test_discriminant_analysis
sklearn.tests.test_discriminant_analysis.test_lda_predict ... ok
sklearn.tests.test_discriminant_analysis.test_lda_priors ... ok
sklearn.tests.test_discriminant_analysis.test_lda_coefs ... ok
sklearn.tests.test_discriminant_analysis.test_lda_transform ... ok
sklearn.tests.test_discriminant_analysis.test_lda_explained_variance_ratio ... FAIL
sklearn.tests.test_discriminant_analysis.test_lda_orthogonality ... ok
sklearn.tests.test_discriminant_analysis.test_lda_scaling ... ok
sklearn.tests.test_discriminant_analysis.test_qda ... ok
sklearn.tests.test_discriminant_analysis.test_qda_priors ... ok
sklearn.tests.test_discriminant_analysis.test_qda_store_covariances ... ok
sklearn.tests.test_discriminant_analysis.test_qda_regularization ... ok
sklearn.tests.test_discriminant_analysis.test_deprecated_lda_qda_deprecation ... ok
sklearn.tests.test_discriminant_analysis.test_covariance ... ok

======================================================================
FAIL: sklearn.tests.test_discriminant_analysis.test_lda_explained_variance_ratio
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/djipey/.local/share/virtualenvs/sk/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/djipey/.local/share/virtualenvs/sk/lib/python3.5/site-packages/sklearn/tests/test_discriminant_analysis.py", line 180, in test_lda_explained_variance_ratio
    clf_lda_eigen.explained_variance_ratio_)
  File "/home/djipey/.local/share/virtualenvs/sk/lib/python3.5/site-packages/numpy/testing/utils.py", line 918, in assert_array_almost_equal
    precision=decimal)
  File "/home/djipey/.local/share/virtualenvs/sk/lib/python3.5/site-packages/numpy/testing/utils.py", line 694, in assert_array_compare
    raise AssertionError(msg)
AssertionError: 
Arrays are not almost equal to 6 decimals

(shapes (3,), (2,) mismatch)
 x: array([  6.037955e-01,   3.962045e-01,   1.787350e-32])
 y: array([ 0.603796,  0.396204])

----------------------------------------------------------------------
Ran 13 tests in 0.112s

FAILED (failures=1)

amueller · 2016-10-17T15:50:56Z

The test fails because it's also wrong for the other solver imho.

My understanding is that the explained_variance_ratio_ refers to how much variance each of the components explains. However, there can only be n_classes - 1 components, as the components are spanned by the means of the classes.

Fix for Eigen and SVD solvers.

JPFrancoia · 2016-10-20T16:26:20Z

You are right (I double-checked, just to be sure). I fixed it for the 2 solvers, and I updated the tests so in the future the length of this attribute will be checked.

amueller · 2016-10-20T16:48:22Z

doc/whats_new.rst

+
+   - Length of `explained_variance_ratio` of
+     :class:`discriminant_analysis.LinearDiscriminantAnalysis`
+     is now the same for Eigen and SVD solvers. (`#7632


I think you should be more explicit by saying that it changed for both and is now min(n_components, n_classes - 1).

amueller · 2016-10-20T16:52:55Z

can you maybe add an attribute n_components_ that is computed in fit and just use that? It should also be used in transform which currently has a kind of nonsensical check.

JPFrancoia · 2016-10-20T17:08:49Z

There is already an attribute self.n_components. It might be ambigious. Maybe I can call it max_components_ ?

amueller · 2016-10-20T17:51:34Z

Not really. We sometimes have attributes with names that are the same as parameters (but with a trailing underscore) if the "real" value is computed from the training data. We could do self._n_components, too, if you want to make it private for now.

LinearDiscriminantAnalysis class. Used to determinate the maximum number of components returned by the class. Also used for the lenght of explained_variance_ratio_. See PR 7632. Fixed some stuff to be more PEP8.

JPFrancoia · 2016-10-23T08:11:25Z

Hum ok, I decided to call it _max_components, because it is really more explicit. This attribute is only used to store the maximum number of components of the classifier, so let's call a cat a cat.

I made it private for now. Let me know if you agree with these changes.

amueller · 2016-10-24T15:54:17Z

hm I'm not sure why you feel _max_components is more apt than _n_components. If n_components < n_classes - 1 it's not really the max, right? But I don't really have a strong opinion, it's private anyhow.

amueller · 2016-10-24T15:55:08Z

LGTM :)

amueller · 2016-10-24T15:55:44Z

(There's one +1 on the current version which is from me. I think @jnothman's doesn't apply any more because there were substantial changes. maybe he has time to review again)

jnothman · 2016-10-25T12:52:00Z

LGTM

…cikit-learn#7632)

JPFrancoia added 2 commits October 10, 2016 23:16

Fix issue scikit-learn#6032. Attribute explained_variance_ratio_ from

6bde65a

LinearDiscriminantAnalysis class will be of length n_components, if provided. If not provided, will have a maximum length of n_classes. The attribute will have the same length, whatever the solver (SVD, Eigen).

Modification of whats_new to warn about the API change for explained_…

14a01cd

…variance_ratio.

JPFrancoia mentioned this pull request Oct 10, 2016

[MRG+1] Correcting length of explained_variance_ratio_, eigen solver #7616

Closed

jnothman requested changes Oct 10, 2016

View reviewed changes

jnothman added this to the 0.18.1 milestone Oct 10, 2016

JPFrancoia added 2 commits October 12, 2016 14:54

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn

b7b1b9b

Modification of the whats_new file. The changes I made are now under

8dd25e9

0.18.1. I created the sub-section "API changes summary" under 0.18.1.

jnothman requested changes Oct 12, 2016

View reviewed changes

Fixed typo :clas: -> :class:

e75e397

jnothman reviewed Oct 13, 2016

View reviewed changes

jnothman approved these changes Oct 13, 2016

View reviewed changes

jnothman changed the title ~~Correcting length of explained_variance_ratio_, eigen solver, final PR~~ [MRG+1] Correcting length of explained_variance_ratio_, eigen solver, final PR Oct 13, 2016

jnothman added the Waiting for Reviewer label Oct 13, 2016

Reformating for line length.

d9591ff

amueller reviewed Oct 13, 2016

View reviewed changes

amueller requested changes Oct 14, 2016

View reviewed changes

explained_variance_ratio_ should have a length of maximum n_classes - 1.

4f84958

Fix for Eigen and SVD solvers.

amueller reviewed Oct 20, 2016

View reviewed changes

Update of whats_new about the length of explained_variance_ratio.

3aa3b1b

Merge https://github.com/scikit-learn/scikit-learn

dbf3586

Creation of the private attribute _max_components in

94e6e8f

LinearDiscriminantAnalysis class. Used to determinate the maximum number of components returned by the class. Also used for the lenght of explained_variance_ratio_. See PR 7632. Fixed some stuff to be more PEP8.

amueller approved these changes Oct 24, 2016

View reviewed changes

jnothman merged commit f260898 into scikit-learn:master Oct 25, 2016

amueller pushed a commit to amueller/scikit-learn that referenced this pull request Oct 25, 2016

[MRG] Correcting length of explained_variance_ratio_, eigen solver (s…

6f58001

…cikit-learn#7632)

amueller pushed a commit to amueller/scikit-learn that referenced this pull request Oct 27, 2016

[MRG] Correcting length of explained_variance_ratio_, eigen solver (s…

71e619a

…cikit-learn#7632)

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

[MRG] Correcting length of explained_variance_ratio_, eigen solver (s…

dcd39a2

…cikit-learn#7632)

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

[MRG] Correcting length of explained_variance_ratio_, eigen solver (s…

9856d09

…cikit-learn#7632)

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

[MRG] Correcting length of explained_variance_ratio_, eigen solver (s…

6263df8

…cikit-learn#7632)

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

[MRG] Correcting length of explained_variance_ratio_, eigen solver (s…

898c8ce

…cikit-learn#7632)

mohamed-ali mentioned this pull request Feb 26, 2018

LDA.explained_variance_ratio_ is of the wrong size #6032

Closed

Uh oh!

Conversation

JPFrancoia commented Oct 10, 2016

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JPFrancoia commented Oct 11, 2016

Uh oh!

jnothman commented Oct 12, 2016

Uh oh!

jnothman commented Oct 12, 2016

Uh oh!

JPFrancoia commented Oct 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Oct 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller left a comment • 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!

JPFrancoia commented Oct 15, 2016

Uh oh!

amueller commented Oct 17, 2016

Uh oh!

JPFrancoia commented Oct 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller commented Oct 20, 2016

Uh oh!

JPFrancoia commented Oct 20, 2016

Uh oh!

amueller commented Oct 20, 2016

Uh oh!

JPFrancoia commented Oct 23, 2016

Uh oh!

amueller commented Oct 24, 2016

Uh oh!

amueller commented Oct 24, 2016

Uh oh!

amueller commented Oct 24, 2016

Uh oh!

jnothman commented Oct 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amueller left a comment •

edited

Loading