Skip to content

[MRG] Add fitted check for kde#16762

Merged
rth merged 5 commits intoscikit-learn:masterfrom
quangngd:check_is_fitted-in-KernalDensity
Apr 5, 2020
Merged

[MRG] Add fitted check for kde#16762
rth merged 5 commits intoscikit-learn:masterfrom
quangngd:check_is_fitted-in-KernalDensity

Conversation

@quangngd
Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Fixes #16613

What does this implement/fix? Explain your changes.

Add fitted check to score_samples and sample for kde

Any other comments?

@quangngd quangngd changed the title Add fitted check for kde [MRG] Add fitted check for kde Mar 25, 2020
Copy link
Copy Markdown
Contributor

@albertcthomas albertcthomas left a comment

Choose a reason for hiding this comment

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

Thanks @quangngd! Could you also add a test checking that a NotFittedError is raised when using sample and score_samples without fitting first? You can check here for an example.

Copy link
Copy Markdown
Contributor

@albertcthomas albertcthomas left a comment

Choose a reason for hiding this comment

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

One small nitpick. Otherwise LGTM. Thanks @quangngd!

Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

@quangngd I see this is your first time contributing, welcome!

assert_allclose(scores, scores_pickled)


def check_estimators_unfitted(name):
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.

This test is not being run currently because it does not have a test_ prefix. We can try this:

@pytest.mark.parametrize('method', ['score_samples', 'sample'])
def test_check_is_fitted(method):
    # Check that predict raises an exception in an unfitted estimator.
    # Unfitted estimators should raise a NotFittedError.
    rng = np.random.RandomState(0)
    X = rng.randn(10, 2)
    kde = KernelDensity()

    with pytest.raises(NotFittedError):
        getattr(kde, method)(X)

Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for working on this, @quangngd

LGTM

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.

Thank you!

@rth rth merged commit 7b5c703 into scikit-learn:master Apr 5, 2020
@quangngd quangngd deleted the check_is_fitted-in-KernalDensity branch April 5, 2020 10:06
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.

check_is_fitted missing in KernelDensity.score_samples

4 participants