8000 FIX validate parameter if `fit` in `KernelDensity` by DessyVV · Pull Request #21430 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

FIX validate parameter if fit in KernelDensity #21430

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.

Sign up for GitHub

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
Merged
merged 5 commits into from
Oct 24, 2021
Merged

FIX validate parameter if fit in KernelDensity #21430

merged 5 commits into from
Oct 24, 2021

Conversation

DessyVV
Copy link
Contributor
@DessyVV DessyVV commented Oct 23, 2021 •
edited
Loading

Reference Issues/PRs

Partially Fixes #21406 by accelerating KDE tests

What does this implement/fix?

Moved the _choose_algorithm call from __init to fit. And changed test_kde_algorithm_metric_choice to raise error when fit method is called.

cc: @ogrisel @LucyJimenez

#DataUmbrella sprint

@amueller
Copy link
Member

Looks good but I think the title doesn't match the PR. This is not about accelerating the test but about not checking parameters in __init__, right?

amueller
amueller previously approved these changes Oct 23, 2021
View reviewed changes
@amueller
Copy link
Member

You actually also need to change test_kde_badargs

@amueller amueller dismissed their stale review October 23, 2021 17:39

ci failure

@DessyVV DessyVV changed the title Accelarate KDE test. Removing parameters tests in init for KDE. Oct 23, 2021
@DessyVV
Copy link
Contributor Author
DessyVV commented Oct 23, 2021

Looks good but I think the title doesn't match the PR. This is not about accelerating the test but about not checking parameters in __init__, right?

@amueller changed the PR message, let me know if the new one makes sense. Thanks!

@ogrisel
Copy link
Member
ogrisel commented Oct 23, 2021 •
edited
Loading

Looks good but I think the title doesn't match the PR. This is not about accelerating the test but about not checking parameters in init, right?

Sorry @DessyVV my bad, I was switching between tables and suggested that title wrongly.

ogrisel
ogrisel approved these changes Oct 23, 2021
View reviewed changes
Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM!

@thomasjpfan you put the "no changelog needed" label. I assume that we will have one changelog entry for all the models at once in the top level changed models section once we merge all the PRs for this issue?

@ogrisel
Copy link
Member
ogrisel commented Oct 23, 2021

Before merging, @DessyVV would you please add an empty commit with the following

git commit --allow-empty

and then write something like the following in the commit message:

Give co-authorship to @LucyJimenez

Co-Authored-By: Lucy Jiménez <name@email.com>

to add co-authorship for this PR to @LucyJimenez?

The name@email.com address needs to be updated with the email address used by Lucy for here github account for this to work.

@thomasjpfan
Copy link
Member

@thomasjpfan you put the "no changelog needed" label. I assume that we will have one changelog entry for all the models at once in the top level changed models section once we merge all the PRs for this issue?

This was by mistake. I think it would be better to add the change log here.

@glemaitre glemaitre changed the title Removing parameters tests in init for KDE. FIX validate parameter if fit in KernelDensity Oct 23, 2021
@glemaitre
Copy link
Member
glemaitre commented Oct 23, 2021 •
edited
Loading

We can solve the remaining failure by adding an entry in the changelog.

Please add an entry to the change log at doc/whats_new/v1.1.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:. For instance, it should look like:

- |Fix| :class:`neighbors.KernelDensity` nows validate input parameters in `fit`
  instead of `__init__`.
  :pr:`21430` by :user:`DessyVV <DessyVV>` and
  :user:`Lucy Jimenez <LucyJimenez >`.

It should in the section:

:mod:`sklearn.neighbors`

@DessyVV DessyVV force-pushed the accelarate-kde-test branch from 72b75c6 to bf35d4d Compare October 24, 2021 07:48
@glemaitre glemaitre merged commit aa675f9 into scikit-learn:main Oct 24, 2021
@glemaitre
Copy link
Member

Thanks @DessyVV @LucyJimenez

@DessyVV DessyVV deleted the accelarate-kde-test branch October 24, 2021 09:44
@DessyVV
Copy link
Contributor Author
DessyVV commented Oct 24, 2021

Thanks @DessyVV @LucyJimenez

Thanks, @glemaitre! Just noticed that it seems like issue #21406 is now closed. Do you if this because of the fix comment in this PR description? If yes, does the issue need to be reopened as this was just one of 12 tasks within the issue?

Thanks!

@glemaitre
Copy link
Member
glemaitre commented Oct 24, 2021 via email

ogrisel pushed a commit to ogrisel/scikit-learn that referenced this pull request Oct 28, 2021
@DessyVV @ogrisel
FIX validate parameter if fit in KernelDensity estimator (scikit-…
e38949e 
…learn#21430)

Co-Authored-By: Lucy Jiménez lucy.jimenez.chem@gmail.com
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@DessyVV @samronsin
FIX validate parameter if fit in KernelDensity estimator (scikit-…
de7e5d1 
…learn#21430)

Co-Authored-By: Lucy Jiménez lucy.jimenez.chem@gmail.com
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@amueller amueller amueller approved these changes

Assignees
No one assigned
Labels
module:neighbors Sprint
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove validation from __init__ and set_params
6 participants
@DessyVV @amueller @ogrisel @thomasjpfan @glemaitre @reshamas
0