MNT Make sample_weight checking more consistent in regression metrics #30886

lucyleeow · Feb 24, 2025

Reference Issues/PRs

What does this implement/fix? Explain your changes.

_check_reg_targets will now perform check_consistent_length on sample_weight as well as y_true and y_pred as well as _check_sample_weight

this means that all array checks are done _check_reg_targets, which means all checks are at the start and means we know who is raising errors relating to inputs
only 2 metrics performed _check_sample_weight but AFAICT other metrics that accept sample_weight would also benefit from this check

Any other comments?

Not sure of what extra tests to add as _check_sample_weight and check_consistent_length are both tested separately, and it seems redundant to check those again in the context of _check_reg_targets.

I guess I could try a few different inputs and check that the result is the same as what _check_sample_weight gives ?

cc @ogrisel

github-actions · Feb 24, 2025

✔️ Linting Passed

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

_{Generated for commit: a5ddd5e. Link to the linter CI: here}

ogrisel · Feb 24, 2025

@lucyleeow there are broken tests with older versions of numpy. Can you please take a look?

lucyleeow · Apr 8, 2025

@ogrisel welp the tests now pass and the CI workflows are so old I can't see them, so I don't know what the previous failure was exactly.

Are you happy with the status @ogrisel or ?

I am not sure this requires a changelog entry. Technically we are adding _check_sample_weight to some metrics, which previously did not have this check. AFAICT the additional checks that users would affect users would be:

sample_weight can only be 1D or scalar
sample_weight cannot be negative

I can add changelogs to all metrics where we added _check_sample_weight to explain the additional checking?

lucyleeow · Apr 28, 2025

@ogrisel gentle ping 🙏

lucyleeow · May 15, 2025

@ogrisel I am thinking of making similar changes to _check_reg_targets - it seems reasonable that _check_sample_weight can be performed in there. It also ensures that check_consistent_length is always performed with y_true, y_pred AND sample_weight, if present, for regression metrics.

But it's probably best to get the okay here first, so gentle ping again 😬

ogrisel

Sorry for the lack of reaction on previous pings. Could you please just add a new test (e.g. test_regression_invalid_sample_weight) in sklearn/metrics/tests/test_common.py with a parametrization similar to the existing test_regression_sample_weight_invariance test to quickly check that all the functions mentioned in the changelog entry actually raise the right exception on invalid sample_weight values?

ogrisel · May 15, 2025

@ogrisel I am thinking of making similar changes to _check_reg_targets - it seems reasonable that _check_sample_weight can be performed in there.

I don't understand this comment. This is precisely what is already implemented in this PR. Did you mean to do something similar for another help function besides _check_reg_targets? If so, which one?

I took a quick look at classification metrics and while some factor input validation logic into the _validate_multiclass_probabilistic_prediction helper, the non-probabilistic metrics do a lot of manual check that could probably be factorized to be more consistent.

lucyleeow · May 15, 2025

I don't understand this comment. This is precisely what is already implemented in this PR.

🤦 I've forgotten what this PR does (the give away was _check_reg_targets). I meant for classification metrics, updating _check_targets to do the check_consistent_length and sample weight check for classification metrics.

ogrisel · May 15, 2025

+1 for a follow-up PR for classification metrics then.

ogrisel · May 15, 2025

Labeling this as array API related, as it will make it easier to use _check_sample_weight in array API PRs (related to metrics or not).

cc @OmarManzoor @StefanieSenger @lesteve @betatim.

lucyleeow · May 16, 2025

Thanks @ogrisel ! Added a test_regression_invalid_sample_weight

OmarManzoor

Thank you for the PR @lucyleeow
A few comments otherwise looks nice

OmarManzoor · May 16, 2025

doc/whats_new/upcoming_changes/sklearn.metrics/30886.fix.rst

@@ -0,0 +1,15 @@
+- Additional `sample_weight` checking has been added to
+  :func:`metrics.mean_absolute_error`, :func:`metrics.mean_pinball_loss`


Suggested change

:func:`metrics.mean_absolute_error`, :func:`metrics.mean_pinball_loss`

:func:`metrics.mean_absolute_error`,

:func:`metrics.mean_pinball_loss`,

Thanks. Reading this again made me realise I was wrong about non-negative checks further down. _check_sample_weight does have a ensure_non_negative added in 1.0 but it was turned off for the 2 regression metrics that currently (on main) use _check_sample_weight.

I've kept this behaviour in this PR but now I think about it, I don't think negative weights make sense (after doing some reading on types of weights and their meaning e.g., sampling/probability weights, analytical weights, frequency weights). I think we should turn on ensure_non_negative for all regression metrics. WDYT?

cc also @ogrisel

I agree that negative sample weights don't make much sense but please check this issue:
#12464

Let's wait for the opinion of @ogrisel

Thanks, I didn't know about that thread, it seems to be a more contentious topic than I originally thought. I would vote for leaving that decision out of this PR.

The only place I've met negative sample weights is in particle physics. They appear in a particular kind of Monte Carlo generator where they simulate some quantum mechanical process where sometimes you get destructive interference and hence want to "subtract" an event. The simple way to use these weights is when you, for example, make a plot of the angular distribution of particles generated in a collision at something like the Large Hadron Collider. When filling the histogram of the angular distribution you use the positive sample weights to increase the content of the corresponding bin. And if the weight is negative you subtract the sample weight from the bin contents. At the end you get the correct angular distribution (the shape of it would be wrong if you didn't subtract things).

Long story short, I think how to treat these samples with negative weights for anything other than histogram'ing is a bit of a mystery (an issue from a previous order of magnitude of issue numbers! :-o), or at least it was ~10yrs ago when I last worked on it.

I'm not sure if we need to forbid them, a long time ago we made an effort with some people from particle physics to try and accommodate their use-case in scikit-learn. But I think we didn't get suuuper far (because it is so tricky/mind bending).

sklearn/metrics/_regression.py

sklearn/metrics/tests/test_regression.py

sklearn/metrics/tests/test_common.py

lucyleeow · May 16, 2025

Thanks @OmarManzoor , changes made. It has raised a question about negative weights though.

betatim

LGTM 🎉 Thanks for the improvement

lucyleeow · May 19, 2025

Lets ignore the negative weights. Are you happy for this to go in @ogrisel / @betatim ?

lucyleeow · May 26, 2025

Gentle ping @betatim @ogrisel , could this go in?

…scikit-learn#30886)

…#30886)

… 1.7 #8457 (#8459) #### Reference Issues/PRs Fixes #8457. #### What does this implement/fix? Explain your changes. Copies the internal v1.6 [_check_reg_targets from sklearn](https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/metrics/_regression.py#L61) into sktime.utils.sklearn._metrics. This is a hotfix for forecasting functions not being compatible with sklearn v1.7 which changed _check_reg_targets' signature in this [MR](scikit-learn/scikit-learn#30886.). See discussion on issue on the longer term fix. This short term fix is probably worth it since _check_reg_targets is called at the start of most forecasting._functions and there is no workaround or kwarg that you can give to workaround the issue. It is a hard TypeError in the signature. #### What should a reviewer concentrate their feedback on? This is a hotfix. I do not love the approach but I do not have time for a more principled approach. #### Did you add any tests for the change? The existing tests should now catch this since the module performance_metrics has changed. I am not happy that this was not detected in the unit tests which is the actual cause of the bug - we skip these performance metrics entirely if the module hasn't changed from our side using our pytest `run_test_module_changed` mark. This should ideally also rerun if dependencies changed but is outside the scope of this MR.

improve checks

30ccc16

github-actions bot added module:metrics module:utils labels Feb 24, 2025

merge main

158ea72

lucyleeow added 3 commits April 11, 2025 19:41

Merge branch 'main' into reg_metrics_checks

76705f4

add whats new

6fc0621

Merge branch 'main' into reg_metrics_checks

85a39ee

Merge branch 'main' into reg_metrics_checks

885a005

ogrisel approved these changes May 15, 2025

View reviewed changes

ogrisel added the Array API label May 15, 2025

lucyleeow added 3 commits May 16, 2025 12:25

add common test

cc34e43

add to test

3e8e0f4

comment

730fd92

Merge branch 'main' into reg_metrics_checks

ecf692f

OmarManzoor reviewed May 16, 2025

View reviewed changes

review

a5ddd5e

betatim approved these changes May 16, 2025

View reviewed changes

lucyleeow added Waiting for Second Reviewer First reviewer is done, need a second one! and removed Waiting for Second Reviewer First reviewer is done, need a second one! labels May 19, 2025

ogrisel merged commit bff3d7d into scikit-learn:main May 29, 2025
41 checks passed

lucyleeow deleted the reg_metrics_checks branch May 29, 2025 08:38

jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request May 30, 2025

MNT Make sample_weight checking more consistent in regression metrics (…

73e2473

…scikit-learn#30886)

elhambbi pushed a commit to elhambbi/scikit-learn that referenced this pull request Jun 1, 2025

MNT Make sample_weight checking more consistent in regression metrics (…

ea1e610

…scikit-learn#30886)

jeremiedbb pushed a commit that referenced this pull request Jun 5, 2025

MNT Make sample_weight checking more consistent in regression metrics (…

f3094eb

…#30886)

	:func:`metrics.mean_absolute_error`, :func:`metrics.mean_pinball_loss`
	:func:`metrics.mean_absolute_error`,
	:func:`metrics.mean_pinball_loss`,

Search code, repositories, users, issues, pull requests...

Uh oh!

MNT Make sample_weight checking more consistent in regression metrics #30886

MNT Make sample_weight checking more consistent in regression metrics #30886

Uh oh!

Conversation

lucyleeow commented Feb 24, 2025

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

github-actions bot commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

ogrisel commented Feb 24, 2025

Uh oh!

lucyleeow commented Apr 8, 2025

Uh oh!

lucyleeow commented Apr 28, 2025

Uh oh!

lucyleeow commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel commented May 15, 2025

Uh oh!

lucyleeow commented May 15, 2025

Uh oh!

ogrisel commented May 15, 2025

Uh oh!

ogrisel commented May 15, 2025

Uh oh!

lucyleeow commented May 16, 2025

Uh oh!

OmarManzoor left a comment

Choose a reason for hiding this comment

Uh oh!

OmarManzoor May 16, 2025

Choose a reason for hiding this comment

Uh oh!

lucyleeow May 16, 2025

Choose a reason for hiding this comment

Uh oh!

OmarManzoor May 16, 2025

Choose a reason for hiding this comment

Uh oh!

lucyleeow May 16, 2025

Choose a reason for hiding this comment

Uh oh!

betatim May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lucyleeow commented May 16, 2025

Uh oh!

betatim left a comment

Choose a reason for hiding this comment

Uh oh!

lucyleeow commented May 19, 2025

Uh oh!

lucyleeow commented May 26, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 24, 2025 •

edited

Loading

lucyleeow commented May 15, 2025 •

edited

Loading

betatim May 16, 2025 •

edited

Loading