MNT Make sample_weight checking more consistent in regression metrics #30886
Conversation
|
@lucyleeow there are broken tests with older versions of numpy. Can you please take a look? |
|
@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
I can add changelogs to all metrics where we added |
|
@ogrisel gentle ping 🙏 |
|
@ogrisel I am thinking of making similar changes to But it's probably best to get the okay here first, so gentle ping again 😬 |
There was a problem hiding this comment.
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?
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 I took a quick look at classification metrics and while some factor input validation logic into the |
🤦 I've forgotten what this PR does (the give away was |
|
+1 for a follow-up PR for classification metrics then. |
|
Labeling this as array API related, as it will make it easier to use |
|
Thanks @ogrisel ! Added a |
There was a problem hiding this comment.
Thank you for the PR @lucyleeow
A few comments otherwise looks nice
| @@ -0,0 +1,15 @@ | ||
| - Additional `sample_weight` checking has been added to | ||
| :func:`metrics.mean_absolute_error`, :func:`metrics.mean_pinball_loss` |
There was a problem hiding this comment.
| :func:`metrics.mean_absolute_error`, :func:`metrics.mean_pinball_loss` | |
| :func:`metrics.mean_absolute_error`, | |
| :func:`metrics.mean_pinball_loss`, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
|
Thanks @OmarManzoor , changes made. It has raised a question about negative weights though. |
Reference Issues/PRs
Ref: #30787 (comment)
What does this implement/fix? Explain your changes.
_check_reg_targetswill now performcheck_consistent_lengthonsample_weightas well asy_trueandy_predas well as_check_sample_weight_check_reg_targets, which means all checks are at the start and means we know who is raising errors relating to inputs_check_sample_weightbut AFAICT other metrics that acceptsample_weightwould also benefit from this checkAny other comments?
Not sure of what extra tests to add as
_check_sample_weightandcheck_consistent_lengthare 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_weightgives ?cc @ogrisel