FIX _safe_divide should handle zero-division with numpy scalar #27312
Conversation
|
Given the failure in the example, it seems that the provided fix is not enough. We can hand up returning a very large value because the denominator is extremely small The next iteration, the numerator and denominator will |
|
I am not sure what it the right fix here to handle the numerical instability: either clip in the safe division because we should not assign such a large value in |
|
Apparently, once we get some
vs. previously
|
|
I also see that we had potentially another strategy in the past: https://github.com/scikit-learn/scikit-learn/pull/26278/files#diff-dac0eef4868535102a7b9aeb319e1501dfbd10f1256f91d532927e12d30bfb15L732 |
|
So this is not enough. The negative gradient vector has some In the previous loss it seems that |
|
I‘ll also have a look, but I need some time. Fortunately, we‘ve plenty of time to fix it. |
|
|
|
@lorentzenchr I have numpy 1.25.1 on mac M1 and I can reproduce the error when I use the main branch. |
|
As an aside (I don't know how much we care to be perfectly honest): code based on |
|
@lorentzenchr @glemaitre @lesteve scikit-learn/sklearn/ensemble/_gb.py Lines 237 to 244 in c634b8a I think we should have indices = np.nonzero(masked_terminal_regions == leaf)[0] # of terminal regionsif we compare with the original code. |
|
Plot if I use on indices = np.nonzero(masked_terminal_regions == leaf)[0]
|
|
Yep this look much what we had. It makes sense also if we where computing gradient on data that we should not have :) |
|
@lesteve @lorentzenchr Instead of the exception catching, we could use the previous trick that |
I think this would work with Pyodide. I am not too sure to which extent we want to support Pyodide quirkiness, as I mentioned above. It feels like not getting some warnings in Pyodide would acceptable but having a gradient boosting algorithm that behave weirdly becau 8000 se the loss becomes NaN or inf is maybe not that great. |
|
So I made the fix of @OmarManzoor and change the error catching as it was previously. |
|
I also added a comment to remember why we are not using |
Don't hold your breath too much though, the link above says there is no plan to support it right now, so that's at least 3 years away. |
There was a problem hiding this comment.
LGTM just some nits.
@glemaitre @OmarManzoor @lesteve Thanks for fixing my bugs.
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
…t-learn#27312) Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Fixing the
mainbranch.This PR should handle the case of zero-division with two numpy scalar.
ping @lorentzenchr @thomasjpfan @OmarManzoor @lesteve
Edit: The CI failure popped up after merging #26278 in the example examples/ensemble/plot_gradient_boosting_regularization.py