Skip to content

ENH add score_samples method in base search CV and add tests - #17478

Merged
adrinjalali merged 9 commits into
scikit-learn:masterfrom
teonbrooks:tb-search-cv
Jun 8, 2020
Merged

adrinjalali merged 9 commits into
scikit-learn:masterfrom
teonbrooks:tb-search-cv

Conversation

@teonbrooks

Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Fixes #12542.
Building on and closes #16275.

What does this implement/fix? Explain your changes.

This adds the ability to call score_samples on the estimator with the best found parameters in *SearchCV instances.

@amueller

amueller commented Jun 6, 2020

Copy link
Copy Markdown
Member

cc @thomasjpfan who reviewed the previous PR

Comment thread sklearn/model_selection/tests/test_search.py Outdated
Comment thread sklearn/model_selection/tests/test_search.py Outdated
@teonbrooks teonbrooks changed the title WIP add score_samples method in base search CV and add tests [MRG] add score_samples method in base search CV and add tests Jun 6, 2020
@amueller

amueller commented Jun 6, 2020

Copy link
Copy Markdown
Member

Please fix the linting issues :)

@thomasjpfan thomasjpfan left a comment

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.

Thank you for the PR @teonbrooks !

Comment thread sklearn/model_selection/tests/test_search.py Outdated
Comment thread sklearn/model_selection/tests/test_search.py Outdated
Comment thread sklearn/model_selection/tests/test_search.py Outdated
Comment thread sklearn/model_selection/_search.py
Comment thread sklearn/model_selection/tests/test_search.py Outdated
Comment thread sklearn/model_selection/tests/test_search.py Outdated

@thomasjpfan thomasjpfan left a comment •

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.

Our linter runs on the diff made by the PR, so the lint issues outside the diff will not make the linter fail.

Given this was during the sprint, I think this is okay.

Comment thread sklearn/model_selection/tests/test_search.py Outdated
Comment thread sklearn/model_selection/tests/test_search.py Outdated
Comment thread sklearn/model_selection/tests/test_search.py Outdated
Comment thread sklearn/model_selection/tests/test_search.py Outdated

@thomasjpfan thomasjpfan left a comment

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.

LGTM Thank you for the PR @teonbrooks !

@thomasjpfan

thomasjpfan commented Jun 6, 2020 •

Copy link
Copy Markdown
Member

Please add an entry to the change log at doc/whats_new/v0.24.rst with the Feature tag. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

Comment thread sklearn/model_selection/tests/test_search.py Outdated
@reshamas

reshamas commented Jun 6, 2020

Copy link
Copy Markdown
Member

#DataUmbrella sprint

fix indenting

@thomasjpfan thomasjpfan left a comment

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.

LGTM

@thomasjpfan thomasjpfan changed the title [MRG] add score_samples method in base search CV and add tests ENH add score_samples method in base search CV and add tests Jun 7, 2020

@adrinjalali adrinjalali left a comment

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.

Otherwise LGTM.

Comment thread sklearn/model_selection/tests/test_search.py Outdated
@adrinjalali
adrinjalali merged commit 5d04910 into scikit-learn:master Jun 8, 2020
@teonbrooks
teonbrooks deleted the tb-search-cv branch June 8, 2020 16:43
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
* add score_samples method in base search CV and add tests

* add tests and change implem

* fix pep8

* address some pr comments

* Update test_search.py

remove deprecation warnings

* address some linting issues

* Update test_search.py

* update whats_new

fix indenting

* update test_search.py

Co-authored-by: Mohamed Maskani <[email protected]>
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
* add score_samples method in base search CV and add tests

* add tests and change implem

* fix pep8

* address some pr comments

* Update test_search.py

remove deprecation warnings

* address some linting issues

* Update test_search.py

* update whats_new

fix indenting

* update test_search.py

Co-authored-by: Mohamed Maskani <[email protected]>
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.

Add a score_samples method to Pipeline and *SearchCV

6 participants