-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
MAINT compatibility with sklearn 1.6 #1104
base: master
Are you sure you want to change the base?
MAINT compatibility with sklearn 1.6 #1104
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few things I noticed.
imblearn/base.py
Outdated
return parse_version( | ||
parse_version(sklearn.__version__).base_version | ||
) >= parse_version("1.6") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably do a try / except
with import Tags
kinda thing though. With a note that "remove this after 1.6 becomes minimum"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the pattern that we document. But actually, I don't know why we are telling to use available_if
. Right now, we could have both _more_tags
and __sklearn_tags__
define.
The available_if
would make sense if you want to avoid a deprecation warning of an error that we could raise in the future if you define both _more_tags
and __sklearn_tags__
. This is something that I mentioned here: scikit-learn/scikit-learn#30257
imblearn/ensemble/_bagging.py
Outdated
if sklearn_version < parse_version("1.6"): | ||
kwargs = {"force_all_finite": False} | ||
else: | ||
kwargs = {"ensure_all_finite": False} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic can go in your fixes.validate_data
imblearn/utils/_tags.py
Outdated
if sklearn_version >= parse_version("1.6"): | ||
from sklearn.utils._tags import InputTags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A try / except around the import (from the public path) makes more sense I think, which shows the exact thing you care about, instead of using the version as a proxy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Tags are not available in a public path if I'm not wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I check the wrong __init__.py
:)
Working on the compatibility with scikit-learn 1.6