[go: up one dir, main page]
Skip to content
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

MAINT use Voting and Stacking support missing values #27710

Merged
merged 6 commits into from
Dec 5, 2023
Merged

MAINT use Voting and Stacking support missing values #27710

merged 6 commits into from
Dec 5, 2023

Conversation

glemaitre
Copy link
Member

While working on #27709, it appears that using DecisionTree in common tests would be better because:

  • they support missing values
  • they handle multilabel classification
  • they handle multioutput regression

By changing the common tests, I came across an error regarding the allow_nan tag for both the Voting and Stacking estimators.

This PR makes 2 changes:

  • use decision tree in common tests to build the heterogeneous ensemble instance
  • fix allow_nan tags for Voting and Stacking estimators

@github-actions GitHub Actions
Copy link
github-actions bot commented Nov 2, 2023 •
edited
Loading

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: eeb3fe4. Link to the linter CI: here

@glemaitre glemaitre mentioned this pull request Nov 2, 2023
Draft
OmarManzoor
OmarManzoor reviewed Dec 4, 2023
View reviewed changes
Copy link
Contributor
@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @glemaitre

sklearn/ensemble/_voting.py Outdated Show resolved Hide resolved
OmarManzoor
OmarManzoor approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor
@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @glemaitre

thomasjpfan
thomasjpfan approved these changes Dec 5, 2023
View reviewed changes
Copy link
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.

LGTM

sklearn/ensemble/_base.py
def _more_tags(self):
try:
allow_nan = all(
_safe_tags(est[1])["allow_nan"] if est[1] != "drop" else True
Copy link
Member

Choose a reason for hiding this comment

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

I agree this is better than main. Unfortunately, our tag system for allowing nans do not work with pipelines.

REF: #27903

@thomasjpfan thomasjpfan merged commit fcdddd5 into scikit-learn:main Dec 5, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OmarManzoor OmarManzoor OmarManzoor approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@glemaitre @thomasjpfan @OmarManzoor