[MRG] Limiting n_components by both n_features and n_samples instead of just n_features #8486

wallygauze · 2017-03-01T21:01:24Z

Reference Issue

Fixes #8484

What does this implement/fix? Explain your changes.

'Handling n_components==None' code in _fit method
Extended (to include n_samples as a limit) relevant ValueError in _fit_full method
Extended ValueError raised for (not 1 <= n_components <= n_features) and ValueError raised for (svd_solver == 'arpack' and n_components == n_features) in _fit_truncated

Documentation changes:

n_component parameter documentation, & mentions elsewhere in parameters section
n_components_ attribute documentation
unrelated (to issue) extra: corrected documentation for explained_variance_ratio_ attribute

codecov · 2017-03-06T05:02:09Z

Codecov Report

Merging #8486 into master will increase coverage by <.01%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master    #8486      +/-   ##
==========================================
+ Coverage   95.48%   95.48%   +<.01%     
==========================================
  Files         342      342              
  Lines       61000    61023      +23     
==========================================
+ Hits        58246    58269      +23     
  Misses       2754     2754

Impacted Files	Coverage Δ
sklearn/decomposition/tests/test_pca.py	`100% <100%> (ø)`	⬆️
sklearn/decomposition/pca.py	`94.55% <91.66%> (+0.05%)`	⬆️
sklearn/ensemble/gradient_boosting.py	`95.79% <0%> (ø)`	⬆️
sklearn/feature_selection/univariate_selection.py	`99.46% <0%> (ø)`	⬆️
sklearn/utils/estimator_checks.py	`93.36% <0%> (+0.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a36c852...df6b90c. Read the comment docs.

lesteve · 2017-03-06T09:04:06Z

You have flake8 failures that you need to understand. Also use a descriptive title please.

wallygauze · 2017-03-07T00:27:44Z

Since my pull request introduces no change in coverage, I believe this codecov fail is irrelevant.

agramfort · 2017-03-07T20:49:37Z

please add a non-regression test. Start from the gist in the issue:

import numpy as np
from .pca import PCA

X = np.array([[-1, -1,3,4,-1, -1,3,4], [-2, -1,5,-1, -1,3,4,2], [-3, -2,1,-1, -1,3,4,1],
[1, 1,4,-1, -1,3,4,2], [2, 1,0,-1, -1,3,4,2], [3, 2,10,-1, -1,3,4,10]])

pca = PCA(n_components=7, svd_solver="arpack")

pca.fit(X)

wallygauze · 2017-03-08T07:48:54Z

While running the tests, I realised that pca.py can't handle the user not entering the number of components if the solver is 'arpack' : with that particular solver, n_components must be strictly less than min(n_samples, n_features), so the default (exactly min(n_samples, n_features)) would fail.

glemaitre · 2017-04-05T14:27:55Z

sklearn/decomposition/pca.py

-        explained is greater than the percentage specified by n_components
-        n_components cannot be equal to n_features for svd_solver == 'arpack'.
+        explained is greater than the percentage specified by n_components.
+        if svd_solver == 'arpack', the number of components must be strictly


You need a capital letter.

glemaitre · 2017-04-05T14:31:07Z

sklearn/decomposition/tests/test_pca.py

+    # matrix X) raise errors.
+    X = np.array([[0, 1, 0], [1, 0, 0]])
    for solver in solver_list:
        for n_components in [-1, 3]:


You should use assert_raises_regex to check also the message return and not only assert_raises.

…ge in docs

wallygauze · 2017-04-07T16:38:13Z

Thank you @glemaitre .

wallygauze · 2017-04-13T17:46:30Z

There was seemingly a problem with Travis until recently, so I am assuming that is why no checks were performed. I am going to open a new pull request, so all checks are run.

glemaitre · 2017-04-13T17:53:59Z

you don't have to close
you should amend and make a push force
that will be enough

wallygauze · 2017-04-13T18:31:38Z

@glemaitre I did that, but it seemed I could no longer reopen this pull-request because amending the last commit rewrote history (which makes sense). Did I misunderstand what you advised? I perhaps should have pushed an empty commit on top. Regardless, I have opened a new pull-request already

glemaitre · 2017-04-13T18:46:44Z

‎You should have reopen the PR and then do the amend and push. I think that if you push somewhere else you cannot reopen. Anyway it is fine. Guillaume Lemaitre INRIA Saclay Ile-de-France / Equipe [email protected] - https://glemaitre.github.io/ ‎

fixed issue 8484

9875d89

wallygauze changed the title ~~Fixed issue 8484~~ [MRG] Fixed issue 8484 Mar 1, 2017

wallygauze changed the title ~~[MRG] Fixed issue 8484~~ [MRG] Fixing issue 8484 Mar 1, 2017

wallygauze force-pushed the pca_8484 branch from e9ae787 to 9875d89 Compare March 6, 2017 05:01

wallygauze changed the title ~~[MRG] Fixing issue 8484~~ [MRG] Limiting n_components by both n_features and n_samples instead of just n_features Mar 6, 2017

dealt with indentation issues flagged by flake8

3544221

Wally added 2 commits March 8, 2017 07:35

code to handle n_components==None with arpack was missing

0517109

added non-regression tests for my previous changes in pca

285f646

minor change: reverted iterator name in test_pca

f7cf9b0

glemaitre reviewed Apr 5, 2017

View reviewed changes

changed AssertRaises to regex variant in test, and minor writing chan…

df6b90c

…ge in docs

wallygauze closed this Apr 13, 2017

wallygauze mentioned this pull request Apr 13, 2017

[MRG] Limiting n_components by both n_features and n_samples instead of just n_features (Recreated PR) #8741

Closed

wallygauze mentioned this pull request Apr 13, 2017

[MRG+2] Limiting n_components by both n_features and n_samples instead of just n_features (Recreated PR) #8742

Merged

Uh oh!

[MRG] Limiting n_components by both n_features and n_samples instead of just n_features #8486

[MRG] Limiting n_components by both n_features and n_samples instead of just n_features #8486

Uh oh!

Conversation

wallygauze commented Mar 1, 2017 • 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!

codecov bot commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lesteve commented Mar 6, 2017

Uh oh!

wallygauze commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort commented Mar 7, 2017

Uh oh!

wallygauze commented Mar 8, 2017

Uh oh!

glemaitre Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

glemaitre Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

wallygauze commented Apr 7, 2017

Uh oh!

wallygauze commented Apr 13, 2017

Uh oh!

glemaitre commented Apr 13, 2017

Uh oh!

wallygauze commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glemaitre commented Apr 13, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wallygauze commented Mar 1, 2017 •

edited

Loading

codecov bot commented Mar 6, 2017 •

edited

Loading

wallygauze commented Mar 7, 2017 •

edited

Loading

wallygauze commented Apr 13, 2017 •

edited

Loading