Skip to content

Conversation

@justinschuster
Copy link

@justinschuster justinschuster commented Dec 31, 2020

Reference Issues/PRs

Fixes #19050.

What does this implement/fix? Explain your changes.

Added conditon

if self.n_components_ not in (n_samples, n_features):

in sklearn/decompostion/_incremental_pca.py

No longer throws warning message when n_conditions are equal to n_samples.

Any other comments?

Non-regression tests:

import sklearn.decomposition as skdecomp
ipca=skdecomp.IncrementalPCA(n_components=5)
ipca.partial_fit(np.random.randn(5,7))
import sklearn.decomposition as skdecomp
ipca=skdecomp.IncrementalPCA(n_components=5)
ipca.fit(np.random.randn(5,7))

Resulted in no warnings.

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

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 :pr: and credit yourself (and other contributors if applicable) with :user:.

Please add a non-regression test that would fail at master but pass in this PR. Basically, we should check that we don't raise the warning.

self.explained_variance_ratio_ = \
explained_variance_ratio[:self.n_components_]
if self.n_components_ < n_features:
if self.n_components_ not in (n_samples, n_features):
Copy link
Member

Choose a reason for hiding this comment

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

I think that a comment is required to mention that as stated by @NicolasHug: "since we already ensure that we have n_components <= (n_samples, n_features)".

@glemaitre glemaitre changed the title FIX Micro bug(warning, not error) in Incremental PCA (#19050) FIX avoid spurious warning in IncrementalPCA due to n_samples == n_components_ Jan 4, 2021
Base automatically changed from master to main January 22, 2021 10:53
@thomasjpfan
Copy link
Member

@justinschuster Are you still interested in working on this PR?

@justinschuster justinschuster deleted the incremental_pca_warning branch May 18, 2021 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Micro bug(warning, not error) in Incremental PCA

3 participants