-
-
Notifications
You must be signed in to change notification settings - Fork 26k
ENH Reduce redundancy in floating type checks for Array API support in _regression.py
#30128
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
ENH Reduce redundancy in floating type checks for Array API support in _regression.py
#30128
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.
I'd also add a note in _check_reg_targets
's docstring that we should probably use the new method. Otherwise LGTM.
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
cc @ogrisel |
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.
Overall, LGTM. Here a few suggestions for further improvement.
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.
There is some possible simplification of the code:
Actually, I do not have the permission to accept my own suggestions or to directly push to your branch. So please review and accept the suggestions first. Could you also please rename the |
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
…_with_floating_dtype`.
I've made the changes. Please let me know if there’s anything else you'd like me to update! |
…ype` doc-strings.
…weight`'s data-type.
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.
LGTM. Thanks @virchan
Thank you everyone for your time! |
…n `_regression.py` (scikit-learn#30128) Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
…n `_regression.py` (#30128) Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
…n `_regression.py` (scikit-learn#30128) Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Reference Issues/PRs
Fixes #30106, and unpauses #29978.
What does this implement/fix? Explain your changes.
This PR introduces a new function,
_check_reg_targets_and_floating_dtype
, to streamline the floating type checks in_regression.py
.The new function integrates the
_find_matching_floating_dtype
logic directly into the_check_reg_targets
function, and has the following signature:To inspect the resulting floating-point data type, users can access the
.dtype
attribute of the returned arrays, e.g.,y_true.dtype
ory_pred.dtype
.Additionally, it passes
xp
to avoid redundant namespace inspection, extending the work done in #30092.The following regression metrics remain unchanged by this PR:
mean_pinball_loss
(covered by ENH add support for Array API tomean_pinball_loss
andexplained_variance_score
#29978)root_mean_squared_error
(not applicable)median_absolute_error
(dependent onpercentile
or_weighted_percentile
)explained_variance_score
(covered by ENH add support for Array API tomean_pinball_loss
andexplained_variance_score
#29978)max_error
(not applicable)_mean_tweedie_deviance
(not applicable)mean_poisson_deviance
(not applicable)mean_gamma_deviance
(not applicable)d2_pinball_score
(dependent onpercentile
or_weighted_percentile
)d2_absolute_error_score
(dependent onpercentile
or_weighted_percentile
)Any other comments?
This is part of the Array API project (#26024).
Ping: @ogrisel
Cc: @adrinjalali, @betatim, @glemaitre, @sqali.