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

[MRG+2] Deprecate reorder parameter in auc #9851

Merged
merged 22 commits into from Oct 17, 2017

Conversation

Projects
None yet
3 participants
@qinhanmin2014
Member

qinhanmin2014 commented Sep 29, 2017

Reference Issue

proposed in #9786 by @lesteve

with this change reorder=False(reorder=True?) in the auc function is not used in our code. I think we should deprecate the reorder=False(reorder=True?) parameter and potentially (up for debate) have some threshold in case there are some small negative values in np.diff(tpr) or np.diff(fpr)

What does this implement/fix? Explain your changes.

(1)add tolerance to roc_auc_score when checking whether x is increasing. (discarded)

(2)improve error message when x is neither increasing nor decreasing.

(3)deprecate reorder parameter. reorder=False is now the default behaviour.

reorder=True is introduced in 9f18586 for roc_auc_score. Now it is not used in roc_auc_score due to #9786.

Any other comments?

cc @jnothman @lesteve

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Sep 29, 2017

Member

I would deprecate reorder in the same PR.

To be honest I am not sure it is worth adding tol. If the user has non increasing fpr (which means he is already in a special case, i.e. not using roc_curve which guarantees that fpr and tpr are increasing) then it is up to him to preprocess fpr and tpr before passing them to auc. The error message could be a bit improved to help the user do the right thing. e.g. by mentioning how many negative np.diff(fpr) there is and what's the most negative values we found.

Member

lesteve commented Sep 29, 2017

I would deprecate reorder in the same PR.

To be honest I am not sure it is worth adding tol. If the user has non increasing fpr (which means he is already in a special case, i.e. not using roc_curve which guarantees that fpr and tpr are increasing) then it is up to him to preprocess fpr and tpr before passing them to auc. The error message could be a bit improved to help the user do the right thing. e.g. by mentioning how many negative np.diff(fpr) there is and what's the most negative values we found.

@qinhanmin2014 qinhanmin2014 changed the title from [MRG] Add tol parameter to avoid unexpected error in auc to [MRG] Add tol parameter and deprecate reorder parameter in auc Sep 29, 2017

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Sep 29, 2017

Member

@lesteve Thanks for the instant reply :)
I have addressed all your comments. Now the PR:
(1)add tolerance to roc_auc_score when checking whether x is increasing.
(2)improve error message when x is neither increasing nor decreasing.
(3)deprecate reorder parameter. reorder=False is now the default behaviour.
If you are sure that we don't need some part of the PR, please give a clear instruction and I'll remove it.

Member

qinhanmin2014 commented Sep 29, 2017

@lesteve Thanks for the instant reply :)
I have addressed all your comments. Now the PR:
(1)add tolerance to roc_auc_score when checking whether x is increasing.
(2)improve error message when x is neither increasing nor decreasing.
(3)deprecate reorder parameter. reorder=False is now the default behaviour.
If you are sure that we don't need some part of the PR, please give a clear instruction and I'll remove it.

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Oct 3, 2017

Member

I would be in favour of removing tol and making the error message more informative.

Member

lesteve commented Oct 3, 2017

I would be in favour of removing tol and making the error message more informative.

qinhanmin2014 added some commits Oct 3, 2017

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 3, 2017

Member

@lesteve Thanks. I have removed parameter tol.
For the erorr message, I have sligtly improved it previously. Now it looks like this:

auc([2, 1, 3, 4], [4, 5, 6, 7])
# x is neither increasing nor decreasing : [2 1 3 4].
# np.diff(x) contains 2 positive values and 1 negative values.

Since x can be either increasing or decreasing, I'm wondering what's the best information to provide.
WDYT? Thanks a lot :)

Member

qinhanmin2014 commented Oct 3, 2017

@lesteve Thanks. I have removed parameter tol.
For the erorr message, I have sligtly improved it previously. Now it looks like this:

auc([2, 1, 3, 4], [4, 5, 6, 7])
# x is neither increasing nor decreasing : [2 1 3 4].
# np.diff(x) contains 2 positive values and 1 negative values.

Since x can be either increasing or decreasing, I'm wondering what's the best information to provide.
WDYT? Thanks a lot :)

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Oct 3, 2017

Member

Maybe the most negative value and the most positive value of np.diff? This way you can see that his how far your array is from being increasing or decreasing.

Member

lesteve commented Oct 3, 2017

Maybe the most negative value and the most positive value of np.diff? This way you can see that his how far your array is from being increasing or decreasing.

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 3, 2017

Member

@lesteve Thanks for the instant reply :) I have updated the error message.

auc([2.5, 1.6, 3.7, 4.8], [5, 6, 7, 8])
# ValueError: x is neither increasing nor decreasing : [ 2.5  1.6  3.7  4.8].
# np.diff(x) contains 2 positive values and 1 negative values.
# The most positive value in np.diff(x) : x[2] - x[1] = 2.1.
# The most negative value in np.diff(x) : x[1] - x[0] = -0.9.
Member

qinhanmin2014 commented Oct 3, 2017

@lesteve Thanks for the instant reply :) I have updated the error message.

auc([2.5, 1.6, 3.7, 4.8], [5, 6, 7, 8])
# ValueError: x is neither increasing nor decreasing : [ 2.5  1.6  3.7  4.8].
# np.diff(x) contains 2 positive values and 1 negative values.
# The most positive value in np.diff(x) : x[2] - x[1] = 2.1.
# The most negative value in np.diff(x) : x[1] - x[0] = -0.9.

qinhanmin2014 added some commits Oct 3, 2017

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 4, 2017

Member

@lesteve Thanks :) Both comments are addressed.

Member

qinhanmin2014 commented Oct 4, 2017

@lesteve Thanks :) Both comments are addressed.

@jnothman

Adjust the title to remove reference to tol?

Show outdated Hide outdated sklearn/metrics/ranking.py
Show outdated Hide outdated sklearn/metrics/ranking.py
Show outdated Hide outdated sklearn/metrics/ranking.py

@qinhanmin2014 qinhanmin2014 changed the title from [MRG] Add tol parameter and deprecate reorder parameter in auc to [MRG] Deprecate reorder parameter and improve error message in auc Oct 9, 2017

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 9, 2017

Member

@jnothman Thanks :) The error message is suggested by @lesteve here. (In fact, I have slightly shortened it just now.) I don't have strong opinion on this. Seems that it might indeed introduce some confusion. But at least the original error message is not right. So do you think "x is neither increasing nor decreasing : {}".format(x) (very similar to the original message) is enough?

Member

qinhanmin2014 commented Oct 9, 2017

@jnothman Thanks :) The error message is suggested by @lesteve here. (In fact, I have slightly shortened it just now.) I don't have strong opinion on this. Seems that it might indeed introduce some confusion. But at least the original error message is not right. So do you think "x is neither increasing nor decreasing : {}".format(x) (very similar to the original message) is enough?

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 9, 2017

Member

Why do we want to allow increasing or decreasing? Why don't we require it to be increasing, and state that there is a negative diff of ...? I think it would be much clearer to the user why this is relevant.

Member

jnothman commented Oct 9, 2017

Why do we want to allow increasing or decreasing? Why don't we require it to be increasing, and state that there is a negative diff of ...? I think it would be much clearer to the user why this is relevant.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 9, 2017

Member

It's not like we cause great harm to the user by asking them to reverse their inputs.

Member

jnothman commented Oct 9, 2017

It's not like we cause great harm to the user by asking them to reverse their inputs.

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 9, 2017

Member

@jnothman Thanks. From my perspective, I agree with you. Allow x to be decreasing seems awkward but the implementation is there. So what's your final decision or do you want me to further investigate the origin of the code?

Member

qinhanmin2014 commented Oct 9, 2017

@jnothman Thanks. From my perspective, I agree with you. Allow x to be decreasing seems awkward but the implementation is there. So what's your final decision or do you want me to further investigate the origin of the code?

@jnothman

So the support for decreasing data seems to be undocumented? (Worth dropping support for that too? Or will it actually affect users?)

@@ -47,13 +47,19 @@ def auc(x, y, reorder=False):
Parameters
----------
x : array, shape = [n]
x coordinates.
x coordinates. These must be either monotonic increasing or monotonic
decreasing.
y : array, shape = [n]
y coordinates.

This comment has been minimized.

@jnothman

jnothman Oct 9, 2017

Member

don't these also need to be correspondingly increasing/decreasing in case of ties?

@jnothman

jnothman Oct 9, 2017

Member

don't these also need to be correspondingly increasing/decreasing in case of ties?

Show outdated Hide outdated sklearn/metrics/ranking.py
@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 9, 2017

Member

@jnothman Thanks for the review :)

So the support for decreasing data seems to be undocumented?

I think the doc does not clarify what situation we support. Neither the increasing case nor the decreasing case.

don't these also need to be correspondingly increasing/decreasing in case of ties?

Currently, when reorder is False, we don't check y and users get different result with different y order. When reorder is True, y is used to break ties for x when sorting. It seems reasonable to me.

auc([1, 2, 2], [2, 3, 4])  # 2.5
auc([1, 2, 2], [2, 4, 3])  # 3.0
auc([1, 2, 2], [2, 3, 4], reorder=True)  # 2.5
auc([1, 2, 2], [2, 4, 3], reorder=True)  # 2.5

I assume this should say if False

At a glance, it seems wrong. But I think the original author wants to tell us that if x is the same (in the case of ties), we need to ensure that y is increasing (the curve is ascending). Anyway, I think it is not friendly to users.

After consideration, my suggestions are
(1)Modify the document and the error message to match current implemetation.
(2)Gradually deprecate parameter reorder(it is not used in any example)
(3)Seems that the support for decreasing x is introduced in 9f18586 by GaelVaroquaux in 2012. It has long been here and we will not need much effort to support the function. So you may need to carefully consider whether to drop the function.

Member

qinhanmin2014 commented Oct 9, 2017

@jnothman Thanks for the review :)

So the support for decreasing data seems to be undocumented?

I think the doc does not clarify what situation we support. Neither the increasing case nor the decreasing case.

don't these also need to be correspondingly increasing/decreasing in case of ties?

Currently, when reorder is False, we don't check y and users get different result with different y order. When reorder is True, y is used to break ties for x when sorting. It seems reasonable to me.

auc([1, 2, 2], [2, 3, 4])  # 2.5
auc([1, 2, 2], [2, 4, 3])  # 3.0
auc([1, 2, 2], [2, 3, 4], reorder=True)  # 2.5
auc([1, 2, 2], [2, 4, 3], reorder=True)  # 2.5

I assume this should say if False

At a glance, it seems wrong. But I think the original author wants to tell us that if x is the same (in the case of ties), we need to ensure that y is increasing (the curve is ascending). Anyway, I think it is not friendly to users.

After consideration, my suggestions are
(1)Modify the document and the error message to match current implemetation.
(2)Gradually deprecate parameter reorder(it is not used in any example)
(3)Seems that the support for decreasing x is introduced in 9f18586 by GaelVaroquaux in 2012. It has long been here and we will not need much effort to support the function. So you may need to carefully consider whether to drop the function.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 9, 2017

Member
Member

jnothman commented Oct 9, 2017

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 10, 2017

Member

@jnothman Thanks :)

but then let's not give a long error message that perplexes the user. If they are surprised to find the data is not ordered, they will have not been using roc_auc_score, and are I suppose capable of diagnosing or sorting themselves.

I agree. The error message now is very similar to the previous one.

And we can check if y is sorted with something like assert all(np.where(diffx == 0, diffy, diffx) >= 0)

Are you sure we want to do so or maybe I misunderstand you? From my perspective, if users provide something like auc([1, 2, 2, 3], [1, 3, 2, 1]), I think it might be better to output 3.5 (current behaviour for reorder=False) instead of raising an error.

Member

qinhanmin2014 commented Oct 10, 2017

@jnothman Thanks :)

but then let's not give a long error message that perplexes the user. If they are surprised to find the data is not ordered, they will have not been using roc_auc_score, and are I suppose capable of diagnosing or sorting themselves.

I agree. The error message now is very similar to the previous one.

And we can check if y is sorted with something like assert all(np.where(diffx == 0, diffy, diffx) >= 0)

Are you sure we want to do so or maybe I misunderstand you? From my perspective, if users provide something like auc([1, 2, 2, 3], [1, 3, 2, 1]), I think it might be better to output 3.5 (current behaviour for reorder=False) instead of raising an error.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 10, 2017

Member
Member

jnothman commented Oct 10, 2017

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 10, 2017

Member

@jnothman Sorry but I don't understand your previous comment. What else do I need to do here? Thanks :)

Member

qinhanmin2014 commented Oct 10, 2017

@jnothman Sorry but I don't understand your previous comment. What else do I need to do here? Thanks :)

@jnothman

otherwise lgtm

Show outdated Hide outdated sklearn/metrics/ranking.py
@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 10, 2017

Member

@jnothman Thanks :) Note added in doc and what's new.

Member

qinhanmin2014 commented Oct 10, 2017

@jnothman Thanks :) Note added in doc and what's new.

@jnothman jnothman changed the title from [MRG] Deprecate reorder parameter and improve error message in auc to [MRG+1] Deprecate reorder parameter in auc Oct 10, 2017

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 10, 2017

Member

Could you please remind me how reordering related to the numerical instability in roc_auc_score?

Member

jnothman commented Oct 10, 2017

Could you please remind me how reordering related to the numerical instability in roc_auc_score?

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 10, 2017

Member

@jnothman Sure :) The main reasons for the numerical instability in roc_auc_score are as follows:
(1) We cannot ensure that fps/fpr is increasing since we use subtraction. Actually fps/fpr is increasing not considering the slight error introduced when doing float calculation.
(2) We set reorder=True and sort the points according to x (fpr). So we sort the points into wrong order and get wrong result from roc_auc_score.
Among the two reasons, the second one is more decisive. Because even with the original implementation, if we set reorder=False, we can get result nearly equal to the right result (not consider the error raised by sanity check in auc).
In #9786, we actually solve the problem in a better way : ensure that fps/fpr is increasing by using accumulation and set reorder=False. In this way, we pass the sanity check in auc and get more precise result.
I illustrate the problem with a plot here. You may have a look if you want to further investigate the problem.

Member

qinhanmin2014 commented Oct 10, 2017

@jnothman Sure :) The main reasons for the numerical instability in roc_auc_score are as follows:
(1) We cannot ensure that fps/fpr is increasing since we use subtraction. Actually fps/fpr is increasing not considering the slight error introduced when doing float calculation.
(2) We set reorder=True and sort the points according to x (fpr). So we sort the points into wrong order and get wrong result from roc_auc_score.
Among the two reasons, the second one is more decisive. Because even with the original implementation, if we set reorder=False, we can get result nearly equal to the right result (not consider the error raised by sanity check in auc).
In #9786, we actually solve the problem in a better way : ensure that fps/fpr is increasing by using accumulation and set reorder=False. In this way, we pass the sanity check in auc and get more precise result.
I illustrate the problem with a plot here. You may have a look if you want to further investigate the problem.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 10, 2017

Member
Member

jnothman commented Oct 10, 2017

@jnothman jnothman changed the title from [MRG+1] Deprecate reorder parameter in auc to [MRG+1?] Deprecate reorder parameter in auc Oct 10, 2017

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 10, 2017

Member

@jnothman Personally, I would prefer to remain reorder parameter. Like the support for decreasing x, they have long been here and we will not need much effort to support them (though I don't think they are useful). The deprecation is proposed by lesteve, you might ping him for more reasons. Or if you are pretty sure, please give a clear instruction and I'll remove the deprecation and leave this PR as a doc improvement :)

Member

qinhanmin2014 commented Oct 10, 2017

@jnothman Personally, I would prefer to remain reorder parameter. Like the support for decreasing x, they have long been here and we will not need much effort to support them (though I don't think they are useful). The deprecation is proposed by lesteve, you might ping him for more reasons. Or if you are pretty sure, please give a clear instruction and I'll remove the deprecation and leave this PR as a doc improvement :)

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 10, 2017

Member
Member

jnothman commented Oct 10, 2017

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Oct 10, 2017

Member

Apologies for taking some time to come back to this one.

In other words, a way to put this in plots:

import matplotlib.pyplot as plt
plt.figure()
plt.plot([0, 1, 1], [0, 0, 1], 'o-')
plt.figure()
# this curve has the two last points swapped
plt.plot([0, 1, 1], [0, 1, 0], 'o-')
plt.show()

figure_1

figure_2

Before #9786 was merged we would be switching from the first curve to the second curve because of floating point noise in x and reorder=True. The areas under the curve of this two curves are very different and this was the reason we were seeing roc_auc_score differences a lot bigger than what you would expect from floating point differences.

In the case we fixed in #9786, fpr was not increasing only for floating point reasons. In fact we were guaranteed that fpr were in the right order because they were computed with decreasing thresholds. reorder=True was an easy way to get rid of the error "x is not increasing" but the wrong thing to do because rearranging the curve as reorder=True does was drastically changing the auc score (as the plots above are trying to illustrate). This is the main reason why I would be in favour of dropping the reorder parameter in the long run.

Member

lesteve commented Oct 10, 2017

Apologies for taking some time to come back to this one.

In other words, a way to put this in plots:

import matplotlib.pyplot as plt
plt.figure()
plt.plot([0, 1, 1], [0, 0, 1], 'o-')
plt.figure()
# this curve has the two last points swapped
plt.plot([0, 1, 1], [0, 1, 0], 'o-')
plt.show()

figure_1

figure_2

Before #9786 was merged we would be switching from the first curve to the second curve because of floating point noise in x and reorder=True. The areas under the curve of this two curves are very different and this was the reason we were seeing roc_auc_score differences a lot bigger than what you would expect from floating point differences.

In the case we fixed in #9786, fpr was not increasing only for floating point reasons. In fact we were guaranteed that fpr were in the right order because they were computed with decreasing thresholds. reorder=True was an easy way to get rid of the error "x is not increasing" but the wrong thing to do because rearranging the curve as reorder=True does was drastically changing the auc score (as the plots above are trying to illustrate). This is the main reason why I would be in favour of dropping the reorder parameter in the long run.

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 10, 2017

Member

@lesteve Thanks a lot for the detailed reply :)
So I think your opinion is that it is better for us to force users to pass increasing/decreasing x so that we can have a sanity check to avoid wrong result, right? Considering that the result from auc will be significantly influenced when x is sorted unexpectedly due to slight floating point error. I think at this point at least you persuade me to do the deprecation. I'll update the reason for deprecation.
ping @jnothman

Member

qinhanmin2014 commented Oct 10, 2017

@lesteve Thanks a lot for the detailed reply :)
So I think your opinion is that it is better for us to force users to pass increasing/decreasing x so that we can have a sanity check to avoid wrong result, right? Considering that the result from auc will be significantly influenced when x is sorted unexpectedly due to slight floating point error. I think at this point at least you persuade me to do the deprecation. I'll update the reason for deprecation.
ping @jnothman

qinhanmin2014 added some commits Oct 10, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 10, 2017

Codecov Report

Merging #9851 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9851      +/-   ##
==========================================
- Coverage   96.17%   96.16%   -0.01%     
==========================================
  Files         336      336              
  Lines       62413    62504      +91     
==========================================
+ Hits        60024    60108      +84     
- Misses       2389     2396       +7
Impacted Files Coverage Δ
sklearn/metrics/ranking.py 98.36% <100%> (+0.04%) ⬆️
sklearn/metrics/tests/test_ranking.py 100% <100%> (ø) ⬆️
sklearn/_build_utils/__init__.py 59.18% <0%> (-2.05%) ⬇️
sklearn/manifold/tests/test_t_sne.py 98.93% <0%> (-1.07%) ⬇️
sklearn/datasets/rcv1.py 30.9% <0%> (-0.58%) ⬇️
sklearn/manifold/t_sne.py 99.19% <0%> (-0.01%) ⬇️
sklearn/datasets/california_housing.py 39.47% <0%> (ø) ⬆️
sklearn/datasets/tests/test_samples_generator.py 100% <0%> (ø) ⬆️
sklearn/datasets/species_distributions.py 28% <0%> (ø) ⬆️
sklearn/decomposition/tests/test_pca.py 100% <0%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update daeb3ad...4c4ea88. Read the comment docs.

codecov bot commented Oct 10, 2017

Codecov Report

Merging #9851 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9851      +/-   ##
==========================================
- Coverage   96.17%   96.16%   -0.01%     
==========================================
  Files         336      336              
  Lines       62413    62504      +91     
==========================================
+ Hits        60024    60108      +84     
- Misses       2389     2396       +7
Impacted Files Coverage Δ
sklearn/metrics/ranking.py 98.36% <100%> (+0.04%) ⬆️
sklearn/metrics/tests/test_ranking.py 100% <100%> (ø) ⬆️
sklearn/_build_utils/__init__.py 59.18% <0%> (-2.05%) ⬇️
sklearn/manifold/tests/test_t_sne.py 98.93% <0%> (-1.07%) ⬇️
sklearn/datasets/rcv1.py 30.9% <0%> (-0.58%) ⬇️
sklearn/manifold/t_sne.py 99.19% <0%> (-0.01%) ⬇️
sklearn/datasets/california_housing.py 39.47% <0%> (ø) ⬆️
sklearn/datasets/tests/test_samples_generator.py 100% <0%> (ø) ⬆️
sklearn/datasets/species_distributions.py 28% <0%> (ø) ⬆️
sklearn/decomposition/tests/test_pca.py 100% <0%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update daeb3ad...4c4ea88. Read the comment docs.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 11, 2017

Member
Member

jnothman commented Oct 11, 2017

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 11, 2017

Member

ping @lesteve
I think I'll wait for the final decision from @jnothman and @lesteve.
Personally, after @lesteve's explanation, I might prefer to deprecate reorder. It seems worthwhile to force users to change their code since the potential error might be significant. Also, another important reason is that, I don't think the reorder parameter is designed for general case, because sometimes users might don't want to use y to break ties (like the case below). So it might be better that users sort x and arrange y themselves to get the result they expect.
index
Thanks for the the detailed instructions from all of you :)

Member

qinhanmin2014 commented Oct 11, 2017

ping @lesteve
I think I'll wait for the final decision from @jnothman and @lesteve.
Personally, after @lesteve's explanation, I might prefer to deprecate reorder. It seems worthwhile to force users to change their code since the potential error might be significant. Also, another important reason is that, I don't think the reorder parameter is designed for general case, because sometimes users might don't want to use y to break ties (like the case below). So it might be better that users sort x and arrange y themselves to get the result they expect.
index
Thanks for the the detailed instructions from all of you :)

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 11, 2017

Member
Member

jnothman commented Oct 11, 2017

@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 11, 2017

Member

@jnothman I have updated a more detailed reason for deprecation. Do you think it is enough for you to remove the ? in the title? Thanks :)

Member

qinhanmin2014 commented Oct 11, 2017

@jnothman I have updated a more detailed reason for deprecation. Do you think it is enough for you to remove the ? in the title? Thanks :)

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Oct 11, 2017

Member

Sure

Member

jnothman commented Oct 11, 2017

Sure

@jnothman jnothman changed the title from [MRG+1?] Deprecate reorder parameter in auc to [MRG+1] Deprecate reorder parameter in auc Oct 11, 2017

@lesteve

Some comments, otherwise LGTM.

Show outdated Hide outdated sklearn/metrics/ranking.py
Show outdated Hide outdated sklearn/metrics/ranking.py
@qinhanmin2014

This comment has been minimized.

Show comment
Hide comment
@qinhanmin2014

qinhanmin2014 Oct 11, 2017

Member

@lesteve Thanks :) Comments addressed.

Member

qinhanmin2014 commented Oct 11, 2017

@lesteve Thanks :) Comments addressed.

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Oct 12, 2017

Member

@jnothman any comments on this one? If I don't hear anything I'll merge it in a few days.

Member

lesteve commented Oct 12, 2017

@jnothman any comments on this one? If I don't hear anything I'll merge it in a few days.

@jnothman jnothman changed the title from [MRG+1] Deprecate reorder parameter in auc to [MRG+2] Deprecate reorder parameter in auc Oct 17, 2017

@jnothman jnothman merged commit a7f8c32 into scikit-learn:master Oct 17, 2017

0 of 4 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
lgtm analysis: Python Running analyses for revisions
Details

@qinhanmin2014 qinhanmin2014 deleted the qinhanmin2014:auc_tol branch Oct 17, 2017

@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Oct 17, 2017

Member

Good stuff @qinhanmin2014!

Member

lesteve commented Oct 17, 2017

Good stuff @qinhanmin2014!

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment