ENH Add Poisson criterion to RandomForestRegressor #19464
Conversation
| .. versionadded:: 0.18 | ||
| Mean Absolute Error (MAE) criterion. |
There was a problem hiding this comment.
Please revert this change, i.e. let this versionadded untouched as is.
| if (self.criterion == "poisson" and np.any(y < 0)): | ||
| raise ValueError("Some value(s) of y are negative which is" | ||
| " not allowed for Poisson regression." | ||
| ) |
There was a problem hiding this comment.
| if (self.criterion == "poisson" and np.any(y < 0)): | |
| raise ValueError("Some value(s) of y are negative which is" | |
| " not allowed for Poisson regression." | |
| ) | |
| if self.criterion == "poisson": | |
| if np.any(y < 0): | |
| raise ValueError("Some value(s) of y are negative which is" | |
| " not allowed for Poisson regression.") | |
| if np.sum(y) <= 0: | |
| raise ValueError("Sum of y is not positive which is " | |
| "necessary for Poisson regression.") |
There was a problem hiding this comment.
I think the later error message would make more sense if we emphasized that the sum of y needs to be strictly positive. Alternatively we can could say that y cannot be uniformly zero.
There was a problem hiding this comment.
Pointing out strict positiveness is good. Note that the above is the exact same error message as in the tree module. If we improve it here, the same change should be applied there.
|
A next step is to add a test similar (copy&paste) to scikit-learn/sklearn/tree/tests/test_tree.py Lines 609 to 614 in b251f3f I don't know if we need more tests. It would be nice to always add "poisson" in tests with "criterion". Some of them are not trivial, it starts with poisson requiring y >= 0... |
|
@pk1130 You need to merge main in order to resolve conflicts. It means that the main branch introduced changes, and this PR modifies the same lines of code. Let me know if you need help. |
|
@pk1130 Do you need help or, in case you're too busy, could someone else take over? |
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
|
@pk1130 There are merge conflicts. You need to merge branch main into this branch and resolve the conflicts. This is best done with the support of a good editor/IDE. |
|
hi all, I've opened up a new PR at #19836. |
|
#19836 was merged. |
Reference Issues/PRs
Contributes to issue #19304
What does this implement/fix? Explain your changes.
RandomForestRegressor.Any other comments?
Need to expand test suite for addition of poisson criterion to
RandomForestRegressor.