[MRG+1] avoid integer overflow by using floats for matthews_corrcoef #9693

sam-s · 2017-09-05T21:15:07Z

Reference Issue

Fixes #9622

What does this implement/fix? Explain your changes.

avoid integer overflow by using floats

…rcoef

jnothman

Could you please add a test?

jnothman · 2017-09-05T22:18:13Z

sklearn/metrics/classification.py

@@ -527,7 +527,8 @@ def matthews_corrcoef(y_true, y_pred, sample_weight=None):
    y_true = lb.transform(y_true)
    y_pred = lb.transform(y_pred)

-    C = confusion_matrix(y_true, y_pred, sample_weight=sample_weight)
+    C = confusion_matrix(y_true, y_pred, sample_weight=sample_weight
+    ).astype(np.float64)        # bug#9622


Better to use C = C.astype... than this awkward style

jnothman

Otherwise LGTM

jnothman · 2017-09-06T14:32:07Z

sklearn/metrics/tests/test_classification.py

+        true_pos = conf_matrix[1,1]
+        false_pos = conf_matrix[1,0]
+        false_neg = conf_matrix[0,1]
+        n_points = float(conf_matrix.sum())


Is this float only for the sake of python 2 division? It's not needed. future.division is in effect.

This can also be determined as len(y_true)

done (the float was more along the lines of "code as documentation" - added emphasis why this is "correct").

jnothman · 2017-09-06T14:38:42Z

sklearn/metrics/tests/test_classification.py

+        return mcc_numerator / np.sqrt(mcc_denominator)
+
+    def random_ys(n_points):
+        x_true = np.random.sample(n_points)


We usually want tests to be deterministic (so we don't get occasional test failures). So please use a np.random.RandomState with a fixed seed to generate data.

jnothman · 2017-09-06T14:39:18Z

sklearn/metrics/classification.py

@@ -528,6 +528,7 @@ def matthews_corrcoef(y_true, y_pred, sample_weight=None):
    y_pred = lb.transform(y_pred)

    C = confusion_matrix(y_true, y_pred, sample_weight=sample_weight)
+    C = C.astype(np.float64)    # Bug#9622
    t_sum = C.sum(axis=1)
    p_sum = C.sum(axis=0)
    n_correct = np.trace(C)


can we use n_samples = len(y_true) to avoid imprecision there?

no, because of sample_weight

good point. Thanks

jnothman · 2017-09-06T14:39:49Z

sklearn/metrics/tests/test_classification.py

+        y_pred = (x_pred > 0.5) * 1.0
+        return y_true, y_pred
+
+    for n_points in [100, 10000, 1000000]:


Is there any reason to try with smaller numbers?

The main reason is that the success at smaller numbers makes it clear that the failure with the big numbers indicates a problem with the stock function rather than with mcc_safe.
IOW, this is a "test of the test".
This is cheap, so I see no reason not to do it.

jnothman · 2017-09-06T14:40:09Z

sklearn/metrics/classification.py

@@ -528,6 +528,7 @@ def matthews_corrcoef(y_true, y_pred, sample_weight=None):
    y_pred = lb.transform(y_pred)

    C = confusion_matrix(y_true, y_pred, sample_weight=sample_weight)
+    C = C.astype(np.float64)    # Bug#9622


the bug number is not needed here, as long as the test fails if this line is removed (which it does).

mathematically the astype is unnecessary and, in fact, could be viewed as dangerous, because it increases a possibility of a round-off errors.
the bug reference explains why we do need it (because python lacks transparent bignums).

I think you mean numpy lacks transparent bignums. I'm pretty sure Python by default does them.

I don't mind this terribly. If we keep it, write "Issue #9622" rather than "Bug#9622".

FWIW, to pass the test I found it sufficient to just replace

t_sum = C.sum(axis=1, dtype=np.float64) p_sum = C.sum(axis=0, dtype=np.float64)

but perhaps in other cases the trace is the problem??

Maybe a slightly less magical change would be to do:

diff --git a/sklearn/metrics/classification.py b/sklearn/metrics/classification.py index 7c4697d..9c92be5 100644 --- a/sklearn/metrics/classification.py +++ b/sklearn/metrics/classification.py @@ -528,15 +528,15 @@ def matthews_corrcoef(y_true, y_pred, sample_weight=None): y_pred = lb.transform(y_pred) C = confusion_matrix(y_true, y_pred, sample_weight=sample_weight) - C = C.astype(np.float64) # Bug#9622 t_sum = C.sum(axis=1) p_sum = C.sum(axis=0) n_correct = np.trace(C) n_samples = p_sum.sum() - cov_ytyp = n_correct * n_samples - np.dot(t_sum, p_sum) - cov_ypyp = n_samples ** 2 - np.dot(p_sum, p_sum) - cov_ytyt = n_samples ** 2 - np.dot(t_sum, t_sum) - mcc = cov_ytyp / np.sqrt(cov_ytyt * cov_ypyp) + n_samples_square = n_samples ** 2 + corr_ytyp = n_correct / n_samples - np.dot(t_sum, p_sum) / n_samples_square + corr_ypyp = 1 - np.dot(p_sum, p_sum) / n_samples_square + corr_ytyt = 1 - np.dot(t_sum, t_sum) / n_samples_square + mcc = corr_ytyp / np.sqrt(corr_ytyt * corr_ypyp) if np.isnan(mcc): return 0.

Also I think that this may be better in terms of floating point precision, manipulating quantities that are of order 1, reduces the likelihood of overflowing.

Note the from __future__ import division ensures that int divisions work as in Python 3.

That seems a decent approach!

jnothman · 2017-09-06T22:48:36Z

flake8 errors:


./sklearn/metrics/tests/test_classification.py:491:33: E231 missing whitespace after ','
        true_pos = conf_matrix[1,1]
                                ^
./sklearn/metrics/tests/test_classification.py:492:34: E231 missing whitespace after ','
        false_pos = conf_matrix[1,0]
                                 ^
./sklearn/metrics/tests/test_classification.py:493:34: E231 missing whitespace after ','
        false_neg = conf_matrix[0,1]

lesteve

Maybe the code can be made simpler and the test as well

lesteve · 2017-09-07T12:53:25Z

sklearn/metrics/classification.py

@@ -528,6 +528,7 @@ def matthews_corrcoef(y_true, y_pred, sample_weight=None):
    y_pred = lb.transform(y_pred)

    C = confusion_matrix(y_true, y_pred, sample_weight=sample_weight)
+    C = C.astype(np.float64)    # Bug#9622


Maybe a slightly less magical change would be to do:

diff --git a/sklearn/metrics/classification.py b/sklearn/metrics/classification.py index 7c4697d..9c92be5 100644 --- a/sklearn/metrics/classification.py +++ b/sklearn/metrics/classification.py @@ -528,15 +528,15 @@ def matthews_corrcoef(y_true, y_pred, sample_weight=None): y_pred = lb.transform(y_pred) C = confusion_matrix(y_true, y_pred, sample_weight=sample_weight) - C = C.astype(np.float64) # Bug#9622 t_sum = C.sum(axis=1) p_sum = C.sum(axis=0) n_correct = np.trace(C) n_samples = p_sum.sum() - cov_ytyp = n_correct * n_samples - np.dot(t_sum, p_sum) - cov_ypyp = n_samples ** 2 - np.dot(p_sum, p_sum) - cov_ytyt = n_samples ** 2 - np.dot(t_sum, t_sum) - mcc = cov_ytyp / np.sqrt(cov_ytyt * cov_ypyp) + n_samples_square = n_samples ** 2 + corr_ytyp = n_correct / n_samples - np.dot(t_sum, p_sum) / n_samples_square + corr_ypyp = 1 - np.dot(p_sum, p_sum) / n_samples_square + corr_ytyt = 1 - np.dot(t_sum, t_sum) / n_samples_square + mcc = corr_ytyp / np.sqrt(corr_ytyt * corr_ypyp) if np.isnan(mcc): return 0.

Also I think that this may be better in terms of floating point precision, manipulating quantities that are of order 1, reduces the likelihood of overflowing.

Note the from __future__ import division ensures that int divisions work as in Python 3.

lesteve · 2017-09-07T13:07:21Z

sklearn/metrics/tests/test_classification.py

+
+    for n_points in [100, 10000, 1000000]:
+        y_true, y_pred = random_ys(n_points)
+        assert_almost_equal(matthews_corrcoef(y_true, y_true), 1.0)


Just checking this with a simple array is a perfectly fine regression test for the bug we are seeing. A simpler example showing the bug:

import numpy as np from sklearn.metrics.classification import matthews_corrcoef n = int(1e5) arr = np.repeat([0., 1.], n) assert matthews_corrcoef(arr, arr) == 1. # multi-class test arr = np.repeat([0., 1., 2.], n) assert matthews_corrcoef(arr, arr) == 1.

For completeness the value of matthews_corrcoef(arr, arr) is 0 (two classes) and 35.32 (multi-class) on master.

given the lack of transparent bignums in numpy, your magic is dangerous.
I don't mind the other change ("manipulating quantities that are of order 1"), but casting to float is critical.

I will gladly add the simple test, but I see no reason to give up the big random one.

…ing astype

sam-s · 2017-09-07T17:12:39Z

what does "MRG+1" mean?

NelleV · 2017-09-07T22:26:57Z

@sam-s It means the pull requests has been approved by one core contributor.

jnothman · 2017-09-07T23:22:18Z

@lesteve I don't entirely get why your solution works. Isn't the largest number n_samples_square, which remains an integer in your code?

lesteve · 2017-09-08T09:39:56Z

@lesteve I don't entirely get why your solution works. Isn't the largest number n_samples_square, which remains an integer in your code?

Integer division behaves as we want for Python3 (i.e. true division) and we have from __future__ import division so that it behaves as we want as well for Python 2. Maybe more related to what you wanted to hear, the overflow was because we were multiplying two covariances (order n^2 * n^2 super roughly). For n^2 to overflow you need 3 billion samples.

sam-s · 2017-09-08T12:59:47Z

in the current patch, n_samples is a float64.

lesteve · 2017-09-08T14:15:00Z

given the lack of transparent bignums in numpy, your magic is dangerous.
I don't mind the other change ("manipulating quantities that are of order 1"), but casting to float is critical.

Although I agree it can be surprising at first I would not call it magic, it is just the behaviour of true division that operands get casted to floats before the division takes place.

I am not going to claim to be a floating point expert, but I would go with my version posted in #9693 (comment).

sam-s · 2017-09-08T15:37:11Z

* Loïc Estève ***@***.***> [2017-09-08 07:16:35 -0700]: > given the lack of transparent bignums in numpy, your magic is dangerous. I don't mind the other change ("manipulating quantities that are of order 1"), but casting to float is critical.

Yes, casting to float is the crux of the matter, and the current version does it via the `dtype=np.float64` argument.

Although I agree it can be surprising at first I would not call it magic, it is just the behaviour of true division that operands get casted to floats before the division takes place.

Yes, but we must ensure that they are correct before the cast. The current version does it.

I am not going to claim to be a floating point expert, but I would go with my version posted in #9693 (comment).

The only advantage of your version over the current one is that you compute `n_samples ** 2` once instead of twice. This is more than made up for by several extra divisions. At any rate, "manipulating quantities that are of order 1" does not matter for floats (by the mere _meaning_ of the term "floating point number").

…

-- Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1504 http://steingoldpsychology.com http://www.childpsy.net http://iris.org.il http://mideasttruth.com http://www.dhimmitude.org http://no2bds.org At war time "salt of the earth" becomes "cannon fodder".

amueller · 2017-09-08T15:50:18Z

At any rate, "manipulating quantities that are of order 1" does not
matter for floats (by the mere meaning of the term "floating point
number").

The exponent can also overflow, though I feel that is very unlikely to happen.

I'm more concerned about C.sum() to overflow (though also unlikely?), which is avoided by casting to float.

amueller · 2017-09-08T15:52:02Z

Either solution seems fine for me.

sam-s · 2017-09-08T16:30:15Z

* Andreas Mueller ***@***.***> [2017-09-08 15:51:57 +0000]: > At any rate, "manipulating quantities that are of order 1" does not > matter for floats (by the mere _meaning_ of the term "floating point > number"). The exponent can also overflow, though I feel that is very unlikely to happen.

The only way it could happen is if sample_weight.sum() overflows, as in sample_weight = np.repeat([1e305], 10000) We can deal with this issue by scaling: sample_weight /= abs(sample_weight.max()) + abs(sample_weight.min()) but it is unclear if this is worth the effort. Thanks.

…

-- Sam Steingold (http://sds.podval.org/) on darwin Ns 10.3.1504 http://steingoldpsychology.com http://www.childpsy.net http://honestreporting.com http://islamexposedonline.com http://www.memritv.org http://camera.org Daddy, what does "format disk c: complete" mean?

jnothman

I'm happy to merge whichever solution

sam-s · 2017-09-10T20:05:15Z

On Sat, Sep 9, 2017 at 7:50 AM, Joel Nothman ***@***.***> wrote: > @jnothman commented on this pull request. > I'm happy to merge whichever solution

Thank you, the current version works.

jnothman · 2017-09-10T23:03:46Z

We still need a second lgtm

amueller · 2017-09-11T20:06:08Z

thanks

…cikit-learn#9693) * Fix bug#9622: avoid integer overflow by using floats for matthews_corrcoef * matthews_corrcoef: cosmetic change requested by jnothman * Add test_matthews_corrcoef_overflow for Bug#9622 * test_matthews_corrcoef_overflow: clean-up and make deterministic * matthews_corrcoef: pass dtype=np.float64 to sum & trace instead of using astype * test_matthews_corrcoef_overflow: add simple deterministic tests

remove outdated comment fix also for FeatureUnion [MRG+2] Limiting n_components by both n_features and n_samples instead of just n_features (Recreated PR) (scikit-learn#8742) [MRG+1] Remove hard dependency on nose (scikit-learn#9670) MAINT Stop vendoring sphinx-gallery (scikit-learn#9403) CI upgrade travis to run on new numpy release (scikit-learn#9096) CI Make it possible to run doctests in .rst files with pytest (scikit-learn#9697) * doc/datasets/conftest.py to implement the equivalent of nose fixtures * add conftest.py in root folder to ensure that sklearn local folder is used rather than the package in site-packages * test doc with pytest in Travis * move custom_data_home definition from nose fixture to .rst file [MRG+1] avoid integer overflow by using floats for matthews_corrcoef (scikit-learn#9693) * Fix bug#9622: avoid integer overflow by using floats for matthews_corrcoef * matthews_corrcoef: cosmetic change requested by jnothman * Add test_matthews_corrcoef_overflow for Bug#9622 * test_matthews_corrcoef_overflow: clean-up and make deterministic * matthews_corrcoef: pass dtype=np.float64 to sum & trace instead of using astype * test_matthews_corrcoef_overflow: add simple deterministic tests TST Platform independent hash collision tests in FeatureHasher (scikit-learn#9710) TST More informative error message in test_preserve_trustworthiness_approximately (scikit-learn#9738) add some rudimentary tests for meta-estimators fix extra whitespace in error message add missing if_delegate_has_method in pipeline don't test tuple pipeline for now only copy list if not list already? doesn't seem to help?

…cikit-learn#9693) * Fix bug#9622: avoid integer overflow by using floats for matthews_corrcoef * matthews_corrcoef: cosmetic change requested by jnothman * Add test_matthews_corrcoef_overflow for Bug#9622 * test_matthews_corrcoef_overflow: clean-up and make deterministic * matthews_corrcoef: pass dtype=np.float64 to sum & trace instead of using astype * test_matthews_corrcoef_overflow: add simple deterministic tests

Fix bug#9622: avoid integer overflow by using floats for matthews_cor…

f951bbf

…rcoef

jnothman reviewed Sep 5, 2017

View reviewed changes

sam-s added 2 commits September 6, 2017 10:17

matthews_corrcoef: cosmetic change requested by jnothman

675e0e7

Add test_matthews_corrcoef_overflow for Bug#9622

eebb584

jnothman reviewed Sep 6, 2017

View reviewed changes

test_matthews_corrcoef_overflow: clean-up and make deterministic

8963d40

jnothman changed the title ~~Fix bug#9622: avoid integer overflow by using floats for matthews_corrcoef~~ [MRG+1] avoid integer overflow by using floats for matthews_corrcoef Sep 6, 2017

jnothman added this to the 0.19.1 milestone Sep 6, 2017

lesteve reviewed Sep 7, 2017

View reviewed changes

sam-s added 2 commits September 7, 2017 11:58

matthews_corrcoef: pass dtype=np.float64 to sum & trace instead of us…

80284db

…ing astype

test_matthews_corrcoef_overflow: add simple deterministic tests

ac01384

jnothman reviewed Sep 9, 2017

View reviewed changes

amueller merged commit 4c61e8b into scikit-learn:master Sep 11, 2017

[MRG+1] avoid integer overflow by using floats for matthews_corrcoef #9693

[MRG+1] avoid integer overflow by using floats for matthews_corrcoef #9693

Conversation

sam-s commented Sep 5, 2017

Reference Issue

What does this implement/fix? Explain your changes.

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-s Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman commented Sep 6, 2017

lesteve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesteve Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-s commented Sep 7, 2017

NelleV commented Sep 7, 2017 • edited by jnothman Loading

jnothman commented Sep 7, 2017

lesteve commented Sep 8, 2017

sam-s commented Sep 8, 2017

lesteve commented Sep 8, 2017

sam-s commented Sep 8, 2017 via email • edited Loading

amueller commented Sep 8, 2017

amueller commented Sep 8, 2017

sam-s commented Sep 8, 2017 via email

jnothman left a comment

Choose a reason for hiding this comment

sam-s commented Sep 10, 2017 via email

jnothman commented Sep 10, 2017

amueller commented Sep 11, 2017

sam-s Sep 6, 2017 •

edited

Loading

lesteve Sep 7, 2017 •

edited

Loading

NelleV commented Sep 7, 2017 •

edited by jnothman

Loading

sam-s commented Sep 8, 2017 via email •

edited

Loading