-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
[WIP] Adds support for sample weights in median_absolute_error #3784
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
Conversation
I am not able to find any out of the bag solution from numpy like it is in weighted mean. |
You can maybe reuse the |
Thanks for bringing that issue to my attention @MechCoder |
@@ -207,7 +234,12 @@ def median_absolute_error(y_true, y_pred): | |||
1.0 | |||
|
|||
""" | |||
import ipdb; ipdb.set_trace() |
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.
This is causing test failures.
I've change the title to WIP. Feel free to change it to MRG when you think you have completed the PR. |
@@ -179,6 +180,7 @@ def mean_squared_error(y_true, y_pred, sample_weight=None): | |||
|
|||
|
|||
def median_absolute_error(y_true, y_pred): | |||
#def median_absolute_error(y_true, y_pred, sample_weight=None): |
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.
Why is this commented out?
Edit: actually this would already been tested by |
Thanks. |
no, go ahead and do it.. |
@MechCoder Thanks will surely do it. I need a small help. Is it possible to continue with this PR by sending to this branch ? |
not unless you have write-access to this fork. You can cherry-pick these commits onto your branch and send a new PR if you prefer. |
@maniteja123 Would you be able to wrap up work on this? |
Hi, sorry couldn't get into this and my college work has started. Will look into this now and get back with my doubts if any. |
The function _weighted_percentile is for a 1-D array (row vector) while the median_absolute_error converts them to column vector. Would it be okay to make them as row vectors and then pass to I will use an example here :
Thanks and please let me know if I am understanding something wrong here. |
Yes, it's okay to ravel and pass them as row vectors. |
Sorry for the delay, but I have trivial doubt. Suppose the sample weights are same, should the value be the median when there is no sample_weight ?
This is the result, if the Also would it be better if I just create a PR first before to make it easy ? |
Yes, it should be the median of the absolute error when there is no sample weight provided. |
Thanks a lot for the reply. But my doubt was the result when the sample weights are all equal. Sorry if I wasn't clear. |
Ah, good point ! One work around for this would be to check if the sum of the sample_weights is even, and then handle this case specifically. If it is not clear, please issue a PR and it would be better for me to comment there. |
The issue is slightly more complex, see #6189 For now, I would say let us write our own implementation independent of |
Oh this means there is a need for stricter version of weighted percentile. Could you elaborate on your suggested way to do it now ? Something like the linear interpolation method given in the Wikipedia link ? Thanks for patiently answering my doubts. |
Superseded by #6217 |
Adds support for sample_weight in median_absolute_error. Issue #3450