8000 [MRG] MAINT Use check_scalar in _BaseVoting by genvalen · Pull Request #22204 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG] MAINT Use check_scalar in _BaseVoting #22204

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 13 commits into from
Jan 31, 2022
Merged

[MRG] MAINT Use check_scalar in _BaseVoting #22204

merged 13 commits into from
Jan 31, 2022

Conversation

genvalen
Copy link
Contributor
@genvalen genvalen commented Jan 13, 2022 •
edited
Loading

Reference Issues/PRs

Addresses #20724 and #21927
#DataUmbrella

What does this implement/fix? Explain your changes.

Summary of changes to voting estimators:

  • Identify scalar parameters in _BaseVoting, VotingClassifier, and VotingRegressor.
  • Add tests to ensure estimators raise proper errors when invalid arguments are passed in.
  • Use the helper function check_scalar from sklearn.utils to validate the parameters.

Test and validation progress:

In both estimators

  • n_jobs (ignore)
  • verbose

In VotingClassifier

  • flatten_transform

References

  1. check_scalar docs
  2. PR #20723

Any other comments?

Please advise correct type/range if different than what I have below. (Thank you):
param: type-> range
n_jobs: int-> (-inf, -1] or [1, int), or None (not including 0)
verbose: int-> [0, inf), or np.bool_
flatten_transform: int-> (-inf, inf), or np.bool_

genvalen
genvalen commented Jan 14, 2022
View reviewed changes
@genvalen genvalen changed the title [WIP] MAINT Use check_scalar in _BaseVoting [MRG MAINT Use check_scalar in _BaseVoting Jan 14, 2022
@genvalen genvalen changed the title [MRG MAINT Use check_scalar in _BaseVoting [MRG] MAINT Use check_scalar in _BaseVoting Jan 14, 2022
ogrisel
ogrisel approved these changes Jan 24, 2022
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.

Apart from the unnecessary checks on n_jobs, LGTM.

genvalen and others added 2 commits January 24, 2022 13:59
@genvalen @ogrisel
Update sklearn/ensemble/tests/test_voting.py
c755f62 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@genvalen @ogrisel
Update sklearn/ensemble/tests/test_voting.py
5a5e1fb 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
glemaitre
glemaitre approved these changes Jan 24, 2022
View reviewed changes
Copy link
Member
@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM

@genvalen
Copy link
Contributor Author

Hi @ogrisel! I believe this one is ready for merge. Unnecessary checks have been removed.

@glemaitre glemaitre merged commit fcbd4e0 into scikit-learn:main Jan 31, 2022
@glemaitre
Copy link
Member

The last failure was addressed in main. LGTM. Merging Thanks @genvalen

@genvalen genvalen mentioned this pull request Feb 1, 2022
Closed
@reshamas reshamas mentioned this pull request Feb 5, 2022
Closed
41 tasks
@genvalen genvalen deleted the voting_estimators_add_check_scalar branch March 10, 2023 20:11
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

@glemaitre glemaitre glemaitre approved these changes

Assignees
No one assigned
Labels
module:ensemble No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@genvalen @glemaitre @ogrisel
0