What kinds of discrepancy between expected and actual do we get if we leave the scaling as it was?

We may just need a higher tolerance in assert_almost_equal because we're now dealing with a threshold-based metric rather than discrete labels.

@jnothman Thanks.
If we use the original scaling value, we have to set decimal=2 to pass the test.
Here is the code to reproduce the test:

n_samples = 50
random_state = check_random_state(0)
y_true = random_state.randint(0, 2, size=(n_samples, ))
y_pred = random_state.randint(0, 2, size=(n_samples, ))
y_score = random_state.random_sample(size=(n_samples,))
rng = np.random.RandomState(0)
sample_weight = rng.randint(1, 10, size=len(y_true))
roc_auc_score(y_true, y_score, sample_weight=sample_weight)
# 0.38036523593708349
roc_auc_score(y_true, y_score, sample_weight=sample_weight*2)
# 0.38036523593708349
roc_auc_score(y_true, y_score, sample_weight=sample_weight*0.3)
# 0.38733004532124787

I checked the implementation and it seems right. The difference seems to be introduced by python when doing floating operations.
So should we remain the original scaling value and set decimal=2?

As long as we don't have any estimators that require scores in (0, 1), we could use integer scores to encourage more stability...?

Integer scores, where scores are likely to be equal, would also be a more challenging test to pass

@jnothman Sorry but I don't quite understand. What do you mean by 'integer scores'? Could you please provide more details? Thanks :)
From my perspective, I can only come up with two solutions:
(1)reduce the precision requirement
(2)keep default precision requirement and choose 'good' scaling value(e.g., 0.5) for sample_weight

Currently we have y_score = random_state.random_sample(size=(n_samples,)) generating floats in (0,1). Instead we could use random_state.randint(10, size=(n_samples,)) to generate ints in [0,9].

@jnothman
Do you mean something like this?

y_true = random_state.randint(0, 2, size=(n_samples, ))
y_score = random_state.randint(0, 10, size=(n_samples, ))
sample_weight = rng.randint(1, 10, size=len(y_true))
roc_auc_score(y_true, y_score, sample_weight=sample_weight)

Such method work for roc_auc_score, but brier_score_loss require scores in (0, 1), so the common test cannot pass in this way.

@jnothman
I got strange test failure when decimal=2 (it can pass locally). Could you please help me? Thanks.
(currently I use decimal=1 to pass the test)

AssertionError: 
Arrays are not almost equal to 2 decimals
roc_auc_score sample_weight is not invariant under scaling
 ACTUAL: 0.38036523593708343
 DESIRED: 0.38733004532124793
>>  raise AssertionError('\nArrays are not almost equal to 2 decimals\nroc_auc_score sample_weight is not invariant under scaling\n ACTUAL: 0.38036523593708343\n DESIRED: 0.38733004532124793')

yes on the face of it that looks weird. assert_allclose is a variant which gives more explicit control of tolerances if you'd rather.

@jnothman Thanks. assert_allclose works. CIs are green. Is this OK for you?

Yes, but we'll wait for another review to be sure I'm not missing something!

atol=1e-2 seems quite high and is only required for roc_auc metrics (all the other metrics pass the tests for the default values i.e. atol=0, rtol=1e-7), do we understand why this is the case?

With scaling=1/17 I get differences that are bigger than 1% (in relative difference) which does not seem like it can be explained by floating point differences ...

E               AssertionError: 
E               Not equal to tolerance rtol=1e-07, atol=0
E               roc_auc_score sample_weight is not invariant under scaling
E               (mismatch 100.0%)
E                x: array(0.3803652359370835)
E                y: array(0.3757664622767263)

@lesteve Thanks for your review.
also ping @jnothman
I traced the whole calculation process of roc_auc_score again. The main reason for the differences lies in function auc in ranking.py. When entering this function, all the parameters (i.e., x and y) are almost the same (difference < 1e-7). But since we introduce very small difference for x, we get different order so we get much bigger difference from np.traz. See the following code snippet:

x1 = [0.6198347107438017, 0.6776859504132231, 0.6776859504132231, 
      0.6776859504132231, 0.6776859504132231, 0.6776859504132231,
      0.74380165289256195, 0.74380165289256195, 0.8925619834710744, 
      0.92561983471074383, 1.0, 1.0, 
      1.0, 1.0, 1.0]
x2 = [0.61983471074380181, 0.6776859504132231, 0.67768595041322344, 
      0.67768595041322321, 0.67768595041322321, 0.67768595041322321,
      0.74380165289256217, 0.74380165289256217, 0.89256198347107418, 
      0.92561983471074349, 0.99999999999999956, 0.99999999999999967,
      0.99999999999999989, 0.99999999999999967, 1.0]
y1 = [0.62096774193548387, 0.62096774193548387, 0.62903225806451613, 
      0.66129032258064513, 0.717741935483871, 0.7338709677419355,
      0.7338709677419355, 0.75806451612903225, 0.75806451612903225,
      0.75806451612903225, 0.75806451612903225, 0.80645161290322576, 
      0.81451612903225812, 0.84677419354838712, 0.94354838709677424]
y2 = [0.62096774193548399, 0.62096774193548399, 0.62903225806451624,
      0.66129032258064524, 0.717741935483871, 0.7338709677419355, 
      0.7338709677419355, 0.75806451612903225, 0.75806451612903225,
      0.75806451612903225, 0.75806451612903225, 0.80645161290322587, 
      0.81451612903225812, 0.84677419354838712, 0.94354838709677424]
x1 = np.array(x1)
x2 = np.array(x2)
y1 = np.array(y1)
y2 = np.array(y2)
np.testing.assert_allclose(x1, x2) #pass
np.testing.assert_allclose(y1, y2) #pass
order = np.lexsort((y1, x1))
x1, y1 = x1[order], y1[order]
order = np.lexsort((y2, x2))
x2, y2 = x1[order], y1[order]
print np.trapz(y1, x1)
# 0.27865902426
print np.trapz(y2, x2)
# 0.275193281792

If we want to solve the problem, we will need to write a more robust sorting function (e.g., regard two float as equal if x1-x2<1e-7) instead of simply calling np.lexsort. Also, we may need to modify the following statement to use np.allclose(..., 0).

optimal_idxs = np.where(np.r_[True,
    np.logical_or(np.diff(fps, 2), np.diff(tps, 2)), True])[0]

We also have tricky ways to access our goal. That is, to round x and y before sorting.

x = np.round(x, 10)
y = np.round(y, 10)
order = np.lexsort((y, x))

We get very similar stable result in this way:

roc_auc_score(y_true, y_score, sample_weight=sample_weight)
# before(unstable):0.38036523593708349
# after(stable):0.38036523591966814

@jnothman I trust you on this one. If you think that atol=1e-2 is fine, then let's do that. Maybe have a special case for auc in the test with a FIXME to show that the higher tolerance is only needed for roc_auc scores?

@jnothman So seems that the reason is clear for this problem (np.lexsort and np.traz in function auc) and I come back to the original solution proposed by you. Do you think we need a seperate test for roc_auc_score here as proposed by @lesteve?
BTW, @jnothman @lesteve I'm suddenly wondering why we need to sort in auc. Seems that we already ensure that x(fpr) and y(tpr) are not decreasing here? If so, this can be sloved easily by setting parameter reorder to False in auc?

Do you think we need a seperate test for roc_auc_score here as proposed by @lesteve?

Just for clarity I was not advocating a completely separate test but more a special case, i.e. something like this:

# FIXME: roc_auc scores are more unstable than other scores
kwargs = {'atol': 1e-2} if 'roc_auc' in metric else {}

for scaling in [2, 0.3]:
    assert_almost_equal(
        np.testing.assert_allclose(
            weighted_score,
            metric(y1, y2, sample_weight=sample_weight * scaling),
            atol=1e-2,
            err_msg=("%s sample_weight is not invariant "
                     "under scaling" % name),
            **kwargs))

@jnothman Thanks for your precious time :) Comments addressed.

Do you really think this belongs here? Do we believe this will change user results often enough to caution them here? This makes it seem like their ROC scores will have suddenly changed... I think "bug fix" is more appropriate in any case.

After consideration, I remove the statement about roc_curve (the change of the result is really small, most time much less than 1e-7) and move the statement about roc_auc_score to bug fix section.

It's not really a regression test

I remove the comment since I now think that it is unnecessary. But from my perspective, this is a regression test (fail on master). Because in master, we cannot ensure that fpr and tpr are increasing because we use subtraction instead of accumulation. Though the error is really small, this is actually the core reason of the issue (it cause the wrong sort and the wrong roc_auc_score).

I feel like the fact that elements are sorted for one random sample isn't a very strong assurance. There are edge cases that could be further tested (such as having repeated thresholds), too, but I'm not sure what reasonable edge cases for this test are.

The test is constructed based on @lesteve's suggestion. I think it hit the edge case (float y_score and sample_weight when some adjacent values of fpr and tpr are the same). Now for adjacent same value, we no longer get something not increasing( e.g., [a+1e-10, a-1e-10] ). I have added a comment.

WDYT? Thanks :)

Will merge when @lesteve approves. Might even be reasonable to include in 0.19.1

@lesteve Thanks for the review and the improvement. I have slightly updated the comment of the test.
Seems that the test at least hit some of the edge case (it fails on master). In such cases, y_score and sample_weight are float, some adjacent values of fpr and tpr are actually the same. In python, the small error introduced when doing float calculation seems unavoidable. In this PR, at least we guarantee that fpr and tpr are increasing and avoid significant error for roc_auc_score. WDYT?

@lesteve Thanks a lot for your help :) I totally agree with all your suggestions and have learnt a lot.

I think this is good to go, merging, thanks a lot!

Another piece of advice while I am at it: chose better names for your branches, you can do better than test-feature-3 surely.

Follow up PRs @qinhanmin2014 if you are up to it (I would rather have two separate PRs in this case):

• with this change reorder=False in the auc function is not used in our code. I think we should deprecate the reorder=False 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)
• in this PR we spotted a place where check_consistent_lengths(X, y) was used where check_consistent_lengths(X, y, sample_weight) should have called it would be good to double-check that this error is not present in some other places in our codebase.

@lesteve @jnothman Thanks a lot :) I'll try taking these issues.

@lesteve
I have opened #9870 to address your second concern since it might need more discussions. Also, it seems that part of it depends on #9828. The PR already have a +1 so if you have time, please have a look at it. Thanks a lot :)