[MRG + 1] Fix gradient boosting overflow and various other float comparison on == #7970

chenhe95 · 2016-12-02T22:10:09Z

Reference Issue

What does this implement/fix? Explain your changes.

Before, the code was using == to compare float values and dividing by "zero (~10e-309)" which caused an overflow.

        if denominator == 0:
            tree.value[leaf, 0, 0] = 0.0
        else:
            tree.value[leaf, 0, 0] = numerator / denominator`

Now I made it so that it's

        if np.isclose(denominator, 0.0):
            tree.value[leaf, 0, 0] = 0.0
        else:
            tree.value[leaf, 0, 0] = numerator / denominator

There are several other instances of this happening, which may cause an error and I want to also address those later on.

In addition, this brings back the numpy.isclose() method which is a standardized way of computing if two float scalars or matrices of arbitrary size are almost close to a threshold.

chenhe95 · 2016-12-02T23:14:50Z

I am wondering how float comparisons were handled before, when there was no isclose() function.
Was it just something like (x - y) < 1e-15 on the spot?

amueller · 2016-12-02T23:55:49Z

@chenhe95 yeah usually you want "close to zero" so you can always do norm < eps

amueller · 2016-12-02T23:57:45Z

sklearn/ensemble/gradient_boosting.py

@@ -511,7 +512,7 @@ def _update_terminal_region(self, tree, terminal_regions, leaf, X, y,
        numerator = np.sum(sample_weight * residual)
        denominator = np.sum(sample_weight * (y - residual) * (1 - y + residual))

-        if denominator == 0.0:
+        if isclose(denominator, 0., rtol=0., atol=np.float64(1e-150)):


silly question but is that a scalar? I feel the code denominator < np.float64(1e-150) easier to understand.

It turned out a lot messier than I had anticipated.
I originally planned to just use isclose(denominator, 0.) which seemed pretty clean, but the default tolerance was only at 1e-8.
I suppose it is good to just do abs(denominator) < 1e-150 here.

chenhe95 · 2016-12-03T01:07:00Z

I also went and did some pep8 housekeeping on the file gradient_boosting.py
The only 2 pep8 violations which were not fixed were

sklearn/ensemble/gradient_boosting.py:1035:20: E712 comparison to True should be 'if cond is True:' or 'if cond:'
sklearn/ensemble/gradient_boosting.py:1446:80: E501 line too long (87 > 79 characters)

Which I wasn't quite sure what to do

(from docstring)
    estimators_ : ndarray of DecisionTreeRegressor, shape = [n_estimators, ``loss_.K``]
        The collection of fitted sub-estimators. ``loss_.K`` is 1 for binary
        classification, otherwise n_classes.

        if presort == True:
            if issparse(X):
                raise ValueError(
                    "Presorting is not supported for sparse matrices.")
            else:
                X_idx_sorted = np.asfortranarray(np.argsort(X, axis=0),
                                                 dtype=np.int32)

And here flake8 suggested doing just if presort: but what if presort was a non-empty list and we only wanted the if statement to pass if presort is literally True?

Let me know what you guys think

raghavrv

Apart from reverting all PEP8 changes unrelated to the PR, this LGTM... Thx!

raghavrv · 2016-12-06T16:36:13Z

sklearn/ensemble/gradient_boosting.py

-        tree.value[leaf, 0, 0] = _weighted_percentile(diff, sample_weight, percentile=50)
+        diff = (y.take(terminal_region, axis=0) -
+                pred.take(terminal_region, axis=0))
+        tree.value[leaf, 0, 0] = \


Could you avoid backslash and rather do

_weighted_percentile(diff sample_weight...)

raghavrv · 2016-12-06T16:38:15Z

sklearn/ensemble/gradient_boosting.py

@@ -375,7 +380,8 @@ def negative_gradient(self, y, pred, sample_weight=None, **kargs):
        if sample_weight is None:
            gamma = stats.scoreatpercentile(np.abs(diff), self.alpha * 100)
        else:
-            gamma = _weighted_percentile(np.abs(diff), sample_weight, self.alpha * 100)
+            gamma = _weighted_percentile(


Why are you cleaning up flake8 issues for code that is not modified in this PR... It creates merged conflicts with other PRs. In general we try to enforce flake8 only for the code that is being modified in the PR... +1 for reverting these changes...

raghavrv · 2016-12-06T16:40:33Z

sklearn/ensemble/gradient_boosting.py

@@ -634,7 +646,8 @@ def _update_terminal_region(self, tree, terminal_regions, leaf, X, y,
        numerator = np.sum(y_ * sample_weight * np.exp(-y_ * pred))
        denominator = np.sum(sample_weight * np.exp(-y_ * pred))

-        if denominator == 0.0:
+        # prevents overflow and division by zero
+        if abs(denominator) < 1e-150:


Instead of 1e-150 you could use np.finfo(np.float32).eps... There is precedent for it here

Hmm.. I am not sure. @raghavrv do you have a strong preference for np.finfo(np.float32).eps or do you think 1e-150 is still fine? I personally prefer 1e-150 because the case where this algorithm was failing at was when denominator was around 1e-309, so I felt that 1e-150 was appropriate since it's about half of 300.

>>> np.finfo(np.float).eps 2.2204460492503131e-16 >>> np.finfo(np.double).eps 2.2204460492503131e-16

It's just that I am kind of worried that those values are too large compared to 1e-150 and I'm not sure if it will cause any rounding errors.

How about .tiny then?

>>> np.finfo(np.float32).tiny 1.1754944e-38

I suppose this seems okay!

I was suggesting np.finfo(np.double).tiny, which is close to e-300 for 32 64 bit ~~and much less for 64 bit systems...~~

>>> np.finfo(np.double).tiny 2.2250738585072014e-308

Thing is if your system is 32 bit, denominator - which is the result of np.sum - can only be as low as (np.finfo(np.float<arch>).tiny) IIUC...

I actually wasn't sure about this, because to the power of -308 seemed a bit too small as well, and it can easily overflow for not very large numerators

>>> 4/np.finfo(np.double).tiny __main__:1: RuntimeWarning: overflow encountered in double_scalars inf

Which was originally why I had 1e-150

raghavrv · 2016-12-06T16:41:07Z

sklearn/ensemble/gradient_boosting.py

@@ -970,7 +985,8 @@ def fit(self, X, y, sample_weight=None, monitor=None):
            self._clear_state()

        # Check input
-        X, y = check_X_y(X, y, accept_sparse=['csr', 'csc', 'coo'], dtype=DTYPE)
+        X, y = check_X_y(
+            X, y, accept_sparse=['csr', 'csc', 'coo'], dtype=DTYPE)


+1 for reverting all pep8 changes unrelated to the PR... :)

I'm ambivalent

jnothman · 2016-12-07T12:17:55Z

Is this meant to be MRG, not WIP?

chenhe95 · 2016-12-07T12:21:20Z

@jnothman I think I am going to revert the flake8 fixes and then set the title to MRG.

chenhe95 · 2016-12-08T22:03:29Z

Thanks for the feedback everyone! I have reverted the flake8 things. Let me know how it looks!

raghavrv

Thanks for the PR!

raghavrv · 2016-12-08T23:34:17Z

Could you make it np.finfo(np.double).tiny instead?

raghavrv · 2016-12-08T23:35:28Z

Sorry I missed your comment here...

raghavrv · 2016-12-08T23:36:42Z

You make a good point about overflow, maybe you could just use 1e-150 then. Ping @jnothman or @amueller for advice!

amueller · 2016-12-09T18:02:14Z

1e-150 is fine. np.finfo(np.double).tiny is too small.

chenhe95 · 2016-12-09T18:15:02Z

Okay, it's reverted back to 1e-150

chenhe95 · 2016-12-13T16:22:41Z

AppVeyor is claiming that the log is empty and failing.

jnothman · 2016-12-14T12:38:18Z

LGTM, thanks

jnothman · 2016-12-14T12:38:48Z

Could you please add a bug fix entry to whats_new.rst? Thanks

amueller · 2016-12-14T16:55:18Z

Hm can we add tests that no warning is raised? Or is that too tricky? Otherwise lgtm.

chenhe95 · 2016-12-14T19:19:03Z

@jnothman I have added to whats_new.rst here's how it looks

@amueller I don't really understand what testing that no warning is raised means, but if it's not too complicated I can certainly add it

amueller · 2016-12-14T19:22:01Z

Oh, there's actually a ValueError in the issue. You should add a test to ensure this value error doesn't happen any more after your fix.

chenhe95 · 2016-12-14T19:43:40Z

I am unsure if the ValueError is easily reproducible since the original reporter of the error said

I am unable to reproduce the behaviour since it happens in a heavily parallelized randomized search

But I am fairly confident that this will fix the ValueError because the float will not be compared to 0.0 using == anymore

amueller · 2016-12-14T19:45:11Z

The point about adding a test is that we don't accidentally introduce the same bug down the road.

chenhe95 · 2016-12-14T20:43:14Z

Okay, I'll see what I can do to come up with some test cases.

jnothman · 2016-12-27T04:53:30Z

Any luck on this, @chenhe95?

chenhe95 · 2016-12-27T05:40:27Z

Hmm.. not really. The last few days of finals have been rough and I have been working on my other CountFeaturizer pull request.
Tomorrow I will have some time and I will see if I can come up with any good test cases, but I think that I have a lot of documentation to go through.

jnothman · 2016-12-27T10:56:25Z

@amueller, this is the sort of thing that I suspect we can only reasonably test by separating out a smaller private helper as a unit and testing that. I am inclined to merge the patch even if we can't build a test with ease.

< F42D div class="d-flex">

raghavrv · 2017-01-05T15:14:55Z

(Travis is failing because of a flake8 issue....) Also can we merge as is? @jnothman @amueller

jnothman · 2017-01-06T05:12:20Z

Waiting for @amueller to voice his opinion on to what extent a test is necessary.

amueller · 2017-03-07T14:21:35Z

LGTM.

raghavrv · 2017-03-07T14:34:51Z

Thanks @chenhe95!!

…arison on == (scikit-learn#7970) * reintroduced isclose() and flake8 fixes to fixes.py * changed == 0.0 to isclose(...) * example changes * changed back to abs() < epsilon * flake8 convention on file * reverted flake8 fixes * reverted flake8 fixes (2) * np.finfo(np.float32).tiny instead of hard coded epsilon 1e-150 * reverted to 1e-150 * whats new modified

reintroduced isclose() and flake8 fixes to fixes.py

06fd1f1

chenhe95 mentioned this pull request Dec 2, 2016

[WIP] Fix gradient boosting overflow #7959

Closed

chenhe95 added 2 commits December 2, 2016 17:22

changed == 0.0 to isclose(...)

01bce69

example changes

ec58cda

amueller reviewed Dec 2, 2016

View reviewed changes

amueller mentioned this pull request Dec 3, 2016

.LabelPropagation alpha = 1 might result in changes of initial labels #7967

Closed

chenhe95 added 2 commits December 2, 2016 19:42

changed back to abs() < epsilon

3e1c5d9

flake8 convention on file

74daa9b

raghavrv suggested changes Dec 6, 2016

View reviewed changes

chenhe95 added 2 commits December 8, 2016 16:54

reverted flake8 fixes

93eb972

reverted flake8 fixes (2)

aef9739

chenhe95 changed the title ~~[WIP] Fix gradient boosting overflow and various other float comparison on ==~~ [MRG] Fix gradient boosting overflow and various other float comparison on == Dec 8, 2016

raghavrv approved these changes Dec 8, 2016

View reviewed changes

np.finfo(np.float32).tiny instead of hard coded epsilon 1e-150

c524142

reverted to 1e-150

b15612c

amueller changed the title ~~[MRG] Fix gradient boosting overflow and various other float comparison on ==~~ [MRG + 1] Fix gradient boosting overflow and various other float comparison on == Dec 14, 2016

whats new modified

0de6133

amueller merged commit 919b4a8 into scikit-learn:master Mar 7, 2017

Przemo10 mentioned this pull request Mar 17, 2017

update fork (#1) #8606

Closed

lorentzenchr mentioned this pull request Jan 10, 2024

FIX divide by zero in line search of GradientBoostingClassifier #28095

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG + 1] Fix gradient boosting overflow and various other float comparison on == #7970

[MRG + 1] Fix gradient boosting overflow and various other float comparison on == #7970

[MRG + 1] Fix gradient boosting overflow and various other float comparison on == #7970

[MRG + 1] Fix gradient boosting overflow and various other float comparison on == #7970

Conversation

Reference Issue

What does this implement/fix? Explain your changes.

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

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