The Wayback Machine - https://web.archive.org/web/20210831065043/https://github.com/scikit-learn/scikit-learn/pull/13960
Skip to content
Sign up
Sign in
Sign up

/ scikit-learn

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MRG+1] Allow sparse input to incremental PCA #13960

Merged
merged 1 commit into from Jun 14, 2019
Merged

[MRG+1] Allow sparse input to incremental PCA #13960

merged 1 commit into from Jun 14, 2019

Conversation

@scottgigante
Copy link
Contributor

@scottgigante scottgigante commented May 27, 2019

Reference Issues/PRs

Fixes #13957.

What does this implement/fix? Explain your changes.

Implements sparse input for IncrementalPCA. IncrementalPCA is by design suited to accepting sparse input; this allows the input to be sparse, and if it is so, converts the data to dense on a batch-wise basis.
@scottgigante scottgigante force-pushed the scottgigante:feature/incremental_pca_sparse_input branch 4 times, most recently from 880a7a3 to 17d7248 May 27, 2019
@abhinavsagar
Copy link
Contributor

@abhinavsagar abhinavsagar commented May 28, 2019

Looks good to me.

@abhinavsagar
abhinavsagar approved these changes May 28, 2019
View changes
@scottgigante scottgigante changed the title Allow sparse input to incremental PCA [MRG] Allow sparse input to incremental PCA May 28, 2019
@jnothman
jnothman reviewed May 28, 2019
View changes
doc/modules/decomposition.rst Outdated Show resolved Hide resolved
sklearn/decomposition/incremental_pca.py Show resolved Hide resolved
sklearn/decomposition/incremental_pca.py Outdated Show resolved Hide resolved
@scottgigante scottgigante force-pushed the scottgigante:feature/incremental_pca_sparse_input branch from 17d7248 to dc276bf May 28, 2019
@scottgigante
scottgigante reviewed May 28, 2019
View changes
sklearn/decomposition/incremental_pca.py Outdated Show resolved Hide resolved
@scottgigante scottgigante changed the title [MRG] Allow sparse input to incremental PCA [WIP] Allow sparse input to incremental PCA May 28, 2019
@jnothman
jnothman reviewed May 28, 2019
View changes
sklearn/decomposition/incremental_pca.py Outdated Show resolved Hide resolved
@NicolasHug
NicolasHug reviewed May 28, 2019
View changes
Copy link
Member

@NicolasHug NicolasHug left a comment

Please add an entry to the change log at doc/whats_new/v*.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:.
sklearn/decomposition/incremental_pca.py Show resolved Hide resolved
sklearn/decomposition/tests/test_incremental_pca.py Outdated Show resolved Hide resolved
sklearn/decomposition/tests/test_incremental_pca.py Outdated Show resolved Hide resolved
@scottgigante scottgigante force-pushed the scottgigante:feature/incremental_pca_sparse_input branch 6 times, most recently from 3afe748 to 9e119f3 May 29, 2019
@scottgigante scottgigante changed the title [WIP] Allow sparse input to incremental PCA [MRG] Allow sparse input to incremental PCA May 29, 2019
@jnothman
Copy link
Member

@jnothman jnothman commented May 30, 2019

If I want to apply partial_fit batchwise to multiple input matrices, I would not be able to run step 2 without step 1.

yes, but if the way you treat sparse matrices is to densify them, you might as well get the user to do that...

@scottgigante
Copy link
Contributor Author

@scottgigante scottgigante commented Jun 1, 2019

If I want to apply partial_fit batchwise to multiple input matrices, I would not be able to run step 2 without step 1.

yes, but if the way you treat sparse matrices is to densify them, you might as well get the user to do that...

A clarification: if the user wants to batchwise densify and fit multiple sparse matrices, then this is not currently possible. Example scenario: you want to fit a single estimator to multiple large mtx files. Alternative solution would be to load all of the files, concatenate the matrices with scipy.sparse.vstack and then apply fit.

@jnothman
Copy link
Member

@jnothman jnothman commented Jun 2, 2019

@scottgigante
Copy link
Contributor Author

@scottgigante scottgigante commented Jun 2, 2019

I would presume that turning the entire sparse matrix to dense at once would be undesirable for memory reasons.

@jnothman
Copy link
Member

@jnothman jnothman commented Jun 3, 2019

@NicolasHug
NicolasHug reviewed Jun 3, 2019
View changes
sklearn/decomposition/tests/test_incremental_pca.py Outdated Show resolved Hide resolved
sklearn/decomposition/tests/test_incremental_pca.py Outdated Show resolved Hide resolved
@scottgigante scottgigante force-pushed the scottgigante:feature/incremental_pca_sparse_input branch from 9e119f3 to 5b45e71 Jun 3, 2019
@scottgigante
Copy link
Contributor Author

@scottgigante scottgigante commented Jun 3, 2019

Note: it's failing tests due to some other unrelated change, I think - failing tests in grid search CV.

@scottgigante scottgigante reopened this Jun 6, 2019
@scottgigante scottgigante force-pushed the scottgigante:feature/incremental_pca_sparse_input branch 2 times, most recently from 28c78a0 to 7ea580c Jun 6, 2019
@scottgigante
Copy link
Contributor Author

@scottgigante scottgigante commented Jun 6, 2019

@NicolasHug @jnothman any further comments?

@jnothman
jnothman reviewed Jun 6, 2019
View changes
Copy link
Member

@jnothman jnothman left a comment

Sorry my review time has been limited. Please add a check that partial_fit still raises an appropriate error when passed sparse X. Otherwise lgtm, thanks!
@NicolasHug
NicolasHug reviewed Jun 7, 2019
View changes
Copy link
Member

@NicolasHug NicolasHug left a comment

As Joel mentioned please test the error in partial_fit for dense input.

Also all the methods that accept/return a sparse X should be changed to array-like or sparse matrix

Otherwise LGTM too!
@scottgigante scottgigante force-pushed the scottgigante:feature/incremental_pca_sparse_input branch from 7ea580c to bf978a8 Jun 12, 2019
@scottgigante
Copy link
Contributor Author

@scottgigante scottgigante commented Jun 12, 2019

@jnothman @NicolasHug should be good to go now.

@NicolasHug
NicolasHug reviewed Jun 12, 2019
View changes
sklearn/decomposition/tests/test_incremental_pca.py Outdated Show resolved Hide resolved
@scottgigante
allow sparse input to incremental PCA
cb5a185
@scottgigante scottgigante force-pushed the scottgigante:feature/incremental_pca_sparse_input branch from bf978a8 to cb5a185 Jun 13, 2019
@NicolasHug
NicolasHug approved these changes Jun 13, 2019
View changes
Copy link
Member

@NicolasHug NicolasHug left a comment

Thanks @scottgigante !
@NicolasHug
NicolasHug approved these changes Jun 13, 2019
View changes
Copy link
Member

@NicolasHug NicolasHug left a comment

Thanks @scottgigante !
@scottgigante scottgigante changed the title [MRG] Allow sparse input to incremental PCA [MRG+1] Allow sparse input to incremental PCA Jun 13, 2019
@jnothman
jnothman approved these changes Jun 14, 2019
View changes
@jnothman jnothman merged commit df7dd83 into scikit-learn:master Jun 14, 2019
16 checks passed
16 checks passed
@lgtm-com
LGTM analysis: C/C++ No code changes detected
Details
@lgtm-com
LGTM analysis: JavaScript No code changes detected
Details
@lgtm-com
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
@codecov
codecov/patch 100% of diff hit (target 96.81%)
Details
@codecov
codecov/project 96.82% (+<.01%) compared to 4a6264d
Details
@azure-pipelines
scikit-learn.scikit-learn Build #20190613.31 succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py35_np_atlas) Linux py35_np_atlas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux pylatest_conda) Linux pylatest_conda succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py35_32) Windows py35_32 succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py37_64) Windows py37_64 succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda) macOS pylatest_conda succeeded
Details
@jnothman
Copy link
Member

@jnothman jnothman commented Jun 14, 2019

Thanks Scott!

@scottgigante
Copy link
Contributor Author

@scottgigante scottgigante commented Jun 14, 2019

Thanks @jnothman and @NicolasHug for the detailed reviews!

@scottgigante scottgigante deleted the scottgigante:feature/incremental_pca_sparse_input branch Jun 14, 2019
koenvandevelde added a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@scottgigante @koenvandevelde
ENH allow sparse input to incremental PCA (scikit-learn#13960)
568534d
@scottgigante scottgigante mentioned this pull request Oct 5, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman

@NicolasHug NicolasHug

@abhinavsagar abhinavsagar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

IncrementalPCA should accept sparse input
4 participants
@scottgigante @abhinavsagar @jnothman @NicolasHug
Morty Proxy This is a proxified and sanitized view of the page, visit original site.