8000 API drop Tags.regressor_tags.multi_label by ogrisel · Pull Request #30373 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

API drop Tags.regressor_tags.multi_label #30373

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.

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

Merged

Conversation

ogrisel
Copy link
Member
@ogrisel ogrisel commented Nov 29, 2024

Follow-up on #29677 discovered while reviewing #30187.

Let's remove the field tags.regressor_tags.multi_label because:

  • it's meaningless;
  • < 8000 li>it's redundant with tags.target_tags.multi_output automatically set by MultioutputMixin for regressors;
  • it does not map to any concept document in our glossary.

Note that the bug was already present in ForestRegressor._more_tags before #29677, but since 1.6 is not released yet, let's fix this before making it officially part of our new Tag API.

@ogrisel ogrisel added Bug Quick Review For PRs that are quick to review API and removed module:ensemble module:utils labels Nov 29, 2024
@ogrisel ogrisel added this to the 1.6 milestone Nov 29, 2024
Copy link
github-actions bot commented Nov 29, 2024

✔️ Linting Passed

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

Generated for commit: 81c3c25. Link to the linter CI: here

@ogrisel
Copy link
Member Author
ogrisel commented Nov 29, 2024

No changelog needed because this tag API has not been released yet.

@ogrisel
Copy link
Member Author
ogrisel commented Nov 29, 2024

/cc @adrinjalali.

@adrinjalali
Copy link
Member
adrinjalali commented Nov 29, 2024

This needs more fixes (tests failing). I'm not sure if the tag is used somewhere though, I wasn't sure about it when I wrote the new tags.

@ogrisel
Copy link
Member Author
ogrisel commented Nov 29, 2024

Yes indeed, I am on it.

@ogrisel
Copy link
Member Author
ogrisel commented Nov 29, 2024

This should be fixed now. As I said in the description, only RandomForestRegressor previously had this tag set to True (in addition to the old "multioutput" set to True via the mixin).

I did not find any code that consumed that tag for regressor (besides the checks selftests).

@adrinjalali adrinjalali enabled auto-merge (squash) November 29, 2024 16:54
@adrinjalali adrinjalali merged commit c2b1e75 into scikit-learn:main Nov 29, 2024
30 checks passed
@adrinjalali adrinjalali added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Nov 29, 2024
@ogrisel ogrisel deleted the remove-regressor_tags.multi_label branch December 2, 2024 09:15
jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Dec 4, 2024
virchan pushed a commit to virchan/scikit-learn that referenced this pull request Dec 9, 2024
csadorf added a commit to csadorf/cuml that referenced this pull request May 16, 2025
csadorf added a commit to csadorf/sklearn-compat that referenced this pull request May 20, 2025
adrinjalali pushed a commit to sklearn-compat/sklearn-compat that referenced this pull request May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Bug No Changelog Needed Quick Review For PRs that are quick to review To backport PR merged in master that need a backport to a release branch defined based on the milestone.
Development

Successfully merging this pull request may close these issues.

2 participants
0