Skip to content

FIX Truncated SVD should raise a NotFittedError if not fitted#16821

Merged
rth merged 1 commit intoscikit-learn:masterfrom
alexitkes:alexitkes/truncated-svd
Apr 2, 2020
Merged

FIX Truncated SVD should raise a NotFittedError if not fitted#16821
rth merged 1 commit intoscikit-learn:masterfrom
alexitkes:alexitkes/truncated-svd

Conversation

@alexitkes
Copy link
Copy Markdown
Contributor

@alexitkes alexitkes commented Apr 1, 2020

Closes #16806

Call check_is_fitted within TruncatedSVD to raise a clear exception if not fitted.

Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @alexitkes , LTGM.

This is fine, there is no need for a test, and the 1 CI job failing does so for an unrelated reason also on master.

Merging.

@rth rth changed the title [WIP] Truncated SVD should raise a NotFittedError if not fitted FIX Truncated SVD should raise a NotFittedError if not fitted Apr 2, 2020
@rth rth merged commit 557218c into scikit-learn:master Apr 2, 2020
Reduced version of X. This will always be a dense array.
"""
X = check_array(X, accept_sparse='csr')
check_is_fitted(self)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we check this before the check_array?

Copy link
Copy Markdown
Member

@rth rth Apr 2, 2020

Choose a reason for hiding this comment

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

Maybe, but I don't think it matter that much. check_is_fitted is indeed a bit faster, so if it's not fitted it'll fail faster. But check_array can also fail for incorrect input, in which case there is no need to check_is_fitted, and it doesn't take ages either.

So overall +1 but I'm not sure it's worth making another PR for it.

gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020
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.

TruncatedSVD.transform does not check if fitted

3 participants