8000 [MRG] Add support for Multimetric BayesSearchCV by QuentinSoubeyran · Pull Request #1030 · scikit-optimize/scikit-optimize · GitHub
[go: up one dir, main page]

Skip to content
This repository was archived by the owner on Feb 28, 2024. It is now read-only.

[MRG] Add support for Multimetric BayesSearchCV #1030

Closed
wants to merge 20 commits into from
Closed

[MRG] Add support for Multimetric BayesSearchCV #1030

wants to merge 20 commits into from

Conversation

QuentinSoubeyran
Copy link
Contributor

As discussed in PR #988 and issue #797, this PR implements support for multimetric scoring to BayesSearchCV. The refit parameter is used to drive the optimization process.

Fixes:

Features:

  • add support for list of dict scoring and callable scoring returning a dict of scores to BayesSearchCV
  • added support for refit to specify the score to use for optimisation. Callable refit (accepted by GridSearchCV) are not supported
  • documented the changes, documented the BayesSearchCV.multimetric_ attribute
  • added simple test for the changes
  • added entry in what's new

I don't have much experience with the code style of scikit-learn/scikit-optimize, feel free to reformat the code as needed.

kernc and others added 10 commits January 11, 2021 08:28
Co-authored-by: Tim Head <betatim@gmail.com>
…alidation._fit_and_score

* Update searchcv.py

Further improvements for kernc previous commits.  iid totally removed to prevent troubles with sklearn utils prettyprinting. prettyprinting looks for params when print to repl. Sklearn 0.24 has changed return value after cv, so i changed dict destructuring(a litttle bit dirty)

* Update searchcv.py

* Revert unrelated changes

* PEP8 format; add comment

* Revert reverting "unrelated changes"

This is required to pass tests/test_searchcv.py
with scikit-learn 0.24+.

Co-authored-by: Kernc <kerncece@gmail.com>
@pep8speaks
Copy link
pep8speaks commented May 7, 2021

Hello @QuentinSoubeyran! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-07-15 17:43:00 UTC

@QuentinSoubeyran
Copy link
Contributor Author

@kernc Here is the PR fixing #797 thanks to your code from #988. I tried to be as complete as possible with tests and docs, but If anything is missing or could be improved, I would be happy to refine this PR :)

Copy link
Contributor
@kernc kernc left a comment

Choose a reason for hiding this comment

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

Thanks and sorry for the wait. See a few notes below.

@QuentinSoubeyran QuentinSoubeyran requested a review from kernc May 12, 2021 09:08
@QuentinSoubeyran
Copy link
Contributor Author

Thank you for the review, no worries for the short wait. I integrated most of your suggestions. For the callable refit, I'm not sure about the proposed change, see my explanation above.

@QuentinSoubeyran
Copy link
Contributor Author
QuentinSoubeyran commented Jun 1, 2021

Is there anything that I should improve preventing a merge @kernc @glouppe ?

@QuentinSoubeyran
Copy link
Contributor Author

@glouppe @kernc Since a 0.9 release is under discussion (in #988 ), is there a chance to get this into the release ? Do I need to improve anything before a merge ?

@Abdelgha-4
Copy link

Please merge this!

@QuentinSoubeyran
Copy link
Contributor Author

@Abdelgha-4
Until this is merged and released, you can install the code from this PR using this instructions: #988 (comment)

@QuentinSoubeyran
Copy link
Contributor Author

Closed in favor of #1062 which should have a cleaner history

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0