Skip to content

[MRG+2] warning for PCA with sparse input#7649

Merged
amueller merged 1 commit intoscikit-learn:masterfrom
giorgiop:pca-sparse-input
Oct 18, 2016
Merged

[MRG+2] warning for PCA with sparse input#7649
amueller merged 1 commit intoscikit-learn:masterfrom
giorgiop:pca-sparse-input

Conversation

@giorgiop
Copy link
Copy Markdown
Contributor

@giorgiop giorgiop commented Oct 12, 2016 •
edited
Loading

Fix for #7642.

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Oct 13, 2016

Huh? If check_array here rejects sparse then why does #7642 (comment) claim it gets as far as subtracting a mean?

I don't think a warning followed by an exception is an improvement; but the exception should be raised and this behaviour tested.

@giorgiop
Copy link
Copy Markdown
Contributor Author

giorgiop commented Oct 14, 2016

I think the point in #7642 is that, since there is an instruction subtracting the mean, the developer did not have in mind to support input sparsity in pca. This is indeed the case and in fact there is a check_arracy to perform exactly that check.

The issue converged to the point that the error is not informative enough and misled people into using sparse input with pca. What do you suggest instead of the additional warning? I don't like it very much either

@amueller
Copy link
Copy Markdown
Member

amueller commented Oct 14, 2016

You can immediately raise an error instead of a warning?

@giorgiop
Copy link
Copy Markdown
Contributor Author

giorgiop commented Oct 17, 2016

Now: raising an error and testing it.

@giorgiop giorgiop changed the title warning for PCA with sparse input [MRG] warning for PCA with sparse input Oct 17, 2016
@amueller
Copy link
Copy Markdown
Member

amueller commented Oct 17, 2016

test failure

@giorgiop giorgiop force-pushed the pca-sparse-input branch 4 times, most recently from 705795c to 952d042 Compare October 18, 2016 04:22
@giorgiop
Copy link
Copy Markdown
Contributor Author

giorgiop commented Oct 18, 2016

Should be fixed now.

GaelVaroquaux
GaelVaroquaux requested changes Oct 18, 2016
View reviewed changes
sklearn/decomposition/tests/test_pca.py Outdated

def test_pca_spase_input():

X = np.random.rand(5, 4)
Copy link
Copy Markdown
Member

@GaelVaroquaux GaelVaroquaux Oct 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to use a seeded RNG, for instance "X = np.random.RandomState(0).rand(5, 4)". We really don't want to leave any non seeded RNG in the test suite.

Copy link
Copy Markdown
Contributor Author

@giorgiop giorgiop Oct 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't believe I forgot this :) Thanks.

@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Oct 18, 2016

+1 aside the minor change I asked for.

@GaelVaroquaux GaelVaroquaux changed the title [MRG] warning for PCA with sparse input [MRG+1] warning for PCA with sparse input Oct 18, 2016
@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Oct 18, 2016

+1. @jnothman : do you still have changes requests, or are you +1?

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Oct 18, 2016

LGTm

@jnothman jnothman changed the title [MRG+1] warning for PCA with sparse input [MRG+2] warning for PCA with sparse input Oct 18, 2016
@amueller amueller merged commit 0198e2c into scikit-learn:master Oct 18, 2016
amueller pushed a commit to amueller/scikit-learn that referenced this pull request Oct 25, 2016
@amueller
warning for PCA with sparse input (scikit-learn#7649)
abfd60d
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
@Sundrique
warning for PCA with sparse input (scikit-learn#7649)
ea57325
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
@paulha
warning for PCA with sparse input (scikit-learn#7649)
1d53bea
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@maskani-moh
warning for PCA with sparse input (scikit-learn#7649)
a6d675c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GaelVaroquaux GaelVaroquaux GaelVaroquaux requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@giorgiop @jnothman @amueller @GaelVaroquaux