[MRG+1] Fix incorrect `predict_proba` for `LogisticRegression` in binary case using `multinomial` parameter. #9939

rwolst · 2017-10-17T10:52:38Z

Reference Issue

Fixes #9889

What does this implement/fix? Explain your changes.

Fixes incorrect predictions when fitting a LogisticRegression model on binary outcomes with multi_class='multinomial'.

Any other comments?

…cikit-learn#9889)

…-learn#9889)

…cikit-learn#9889)

TomDLT

LGTM, only nitpicks

TomDLT · 2017-10-18T16:27:07Z

doc/whats_new/v0.20.rst

+- Fixed a bug in :class:`linear_model.LogisticRegression` where when using the
+  parameter ``multi_class='multinomial'``, the ``predict_proba`` method was
+  returning incorrect probabilities in the case of binary outcomes.
+  :issue:`9889`. By user `rwolst`.


Please use the syntax:

:issue:`9889` by :user:`rwolst`.

Please use this issue number, not the original issue. Easier to check logs are complete

Is that from your 0.19.1 experience? I would say that, as a random user, the original issue (rather than the PR) is generally a better source of understanding what the problem was, so I would be mildly in favor of using the original issue in the whats_new entry. The PR is better to understand how the problem was fixed.

No, for something like the 0.19.0 release I tried to check that the changeloga included everything merged that wasn't docs, CI, etc. That is easier with pr#s here. If GitHub had an API to get the PRs closing an issue for a particular issue this would not be difficult...

TomDLT · 2017-10-18T16:31:06Z

sklearn/linear_model/logistic.py

@@ -1102,13 +1102,18 @@ class LogisticRegression(BaseEstimator, LinearClassifierMixin,
        Coefficient of the features in the decision function.

        `coef_` is of shape (1, n_features) when the given problem
-        is binary.
+        is binary and in the case when `multi_class='multinomial'`, then


This is a bit verbose, even though it is a matter of taste.
What about:

`coef_` is of shape (1, n_features) when the given problem is binary. In particular, when `multi_class='multinomial'`, `coef_` corresponds to outcome 1 (True) and `-coef_` corresponds to outcome 0 (False).

TomDLT · 2017-10-18T16:35:18Z

sklearn/linear_model/logistic.py


    intercept_ : array, shape (1,) or (n_classes,)
        Intercept (a.k.a. bias) added to the decision function.

        If `fit_intercept` is set to False, the intercept is set to zero.
-        `intercept_` is of shape(1,) when the problem is binary.
+        `intercept_` is of shape(1,) when the problem is binary and in the


What about:

`intercept_` is of shape(1,) when the problem is binary, and when `multi_class='multinomial'`, it corresponds to outcome 1 (True), while `-intercept` corresponds to outcome 0 (False).

rwolst · 2017-10-26T09:58:27Z

@TomDLT I agree with your comments, I will update the pull request.

On top of the merge, added the changes suggested in pull request.

lesteve · 2017-11-08T15:06:49Z

Can you add a non-regression test please?

TomDLT · 2017-11-08T15:14:25Z

doc/whats_new/v0.20.rst

+- Fixed a bug in :class:`linear_model.LogisticRegression` where when using the
+  parameter ``multi_class='multinomial'``, the ``predict_proba`` method was
+  returning incorrect probabilities in the case of binary outcomes.
+  :issue:`9939` by user `rwolst`.


Please use the following syntax (with the two colons :), which will render as a link to your GitHub profile:

:user:`rwolst`

rwolst · 2017-11-08T15:26:09Z

@lesteve Maybe I am mistaken about a non-regression test, but I think that is what test_ovr_multinomial_iris_binary() is i.e. it failed for master but passes for the new branch.

lesteve · 2017-11-08T15:28:54Z

@lesteve Maybe I am mistaken about a non-regression test, but I think that is what test_ovr_multinomial_iris_binary() is i.e. it failed for master but passes for the new branch.

Sorry I must have missed it in the diff somehow.

jnothman

Otherwise LGTM

jnothman · 2017-11-08T22:07:58Z

doc/whats_new/v0.20.rst

+- Fixed a bug in :class:`linear_model.LogisticRegression` where when using the
+  parameter ``multi_class='multinomial'``, the ``predict_proba`` method was
+  returning incorrect probabilities in the case of binary outcomes.
+  :issue:`9939` by :user: `rwolst`.


No space between colon and backtick please. You're also welcome to include your real name

@rwolst you can find an example of the syntax to use in the previous entry.

Ok, will fix this in next commit.

lesteve

Some small comments

lesteve · 2017-11-09T10:28:07Z

sklearn/linear_model/logistic.py

            return super(LogisticRegression, self)._predict_proba_lr(X)
+        elif self.coef_.shape[0] == 1:


I find it slightly clearer to check the shape of the decision function, i.e. something like:

else: decision == self.decision_function(X) decision_2d = np.c_[-decision, decision] if decision.ndim == 1 else decision return softmax(decision_2d, copy=False)

lesteve · 2017-11-09T10:55:05Z

sklearn/linear_model/tests/test_logistic.py

@@ -565,6 +565,38 @@ def test_ovr_multinomial_iris():
        assert_equal(scores.shape, (3, n_cv, 10))


+def test_ovr_multinomial_iris_binary():


This seems like a valid regression test but I feel something like this would be simpler:

import numpy as np from sklearn.linear_model import LogisticRegression from sklearn.datasets import make_classification X, y = make_classification() clf = LogisticRegression(multi_class='multinomial', solver='saga') clf.fit(X, y) decision = clf.decision_function(X) proba = clf.predict_proba(X) expected_proba_class_1 = (np.exp(decision) / (np.exp(-decision) + np.exp(decision))) expected_proba = np.c_[1 - expected_proba_class_1, expected_proba_class_1] np.testing.assert_allclose(proba, expected_proba)

Basically you only check the relationship between decision_function and predict_proba rather than some consequences further down the line.

do you want these addressed or should we merge given the two +1? (I haven't looked at it myself). @rwolst are you still working on this?

I agree the @lesteve regression test is simpler and will add that. Do you want to keep the old regression test as it still may be useful?

jnothman · 2018-01-01T21:44:19Z

happy with just @lesteve's test, thanks

jnothman · 2018-01-06T20:50:36Z

Thanks a lot, @rwolst. Merging!

jnothman · 2018-08-19T23:17:18Z

As per #11476 (comment) and subsequent discussion, we are reconsidering this. We're no longer persuaded that this is the right fix, but rather that we should always use the binary logistic formulation when the input is binary. This would be consistent with the existing behaviour of liblinear (and more generally with multi_class='ovr'), and with the literal meaning of multi_class=* (i.e. that setting shouldn't affect the binary case). Please let us know ASAP, @rwolst, if you have a basis for disagreeing with that conclusion. Thanks.

rwolst added 4 commits October 16, 2017 11:36

Incorrect multinomial logistic regression predict_proba test added (s…

8939713

…cikit-learn#9889)

Fixed incorrect multinomial logistic regression predict_proba (scikit…

4ecac9e

…-learn#9889)

Updated what's new for multinomial logistic regression predictions (s…

ae22f35

…cikit-learn#9889)

Updated doc string for coef_ and intercept_ (scikit-learn#9889)

70e46b4

TomDLT approved these changes Oct 18, 2017

View reviewed changes

TomDLT changed the title ~~[MRG] Fix incorrect predict_proba for LogisticRegression in binary case using multinomial parameter.~~ [MRG+1] Fix incorrect predict_proba for LogisticRegression in binary case using multinomial parameter. Oct 18, 2017

Merge branch 'master' into lr_multinomial_bug_fix

65072f7

On top of the merge, added the changes suggested in pull request.

TomDLT reviewed Nov 8, 2017

View reviewed changes

Fixed :user: formatting in whats new

fd59597

jnothman approved these changes Nov 8, 2017

View reviewed changes

lesteve reviewed Nov 9, 2017

View reviewed changes

qinhanmin2014 mentioned this pull request Nov 23, 2017

[MRG+2] discrete branch: add an example for KBinsDiscretizer #10192

Merged

Merge branch 'master' into lr_multinomial_bug_fix

e9927e4

rwolst added 2 commits January 6, 2018 10:48

Merge branch 'master' into lr_multinomial_bug_fix

16a3179

Simplified regression test

fb88a0f

jnothman merged commit 4dafa52 into scikit-learn:master Jan 6, 2018

jnothman mentioned this pull request Aug 19, 2018

[MRG] Change default solver in LogisticRegression #11476

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] Fix incorrect `predict_proba` for `LogisticRegression` in binary case using `multinomial` parameter. #9939

[MRG+1] Fix incorrect `predict_proba` for `LogisticRegression` in binary case using `multinomial` parameter. #9939

rwolst commented Oct 17, 2017

TomDLT left a comment

TomDLT Oct 18, 2017

jnothman Oct 26, 2017

lesteve Oct 27, 2017

jnothman Oct 28, 2017

TomDLT Oct 18, 2017

TomDLT Oct 18, 2017

rwolst commented Oct 26, 2017

lesteve commented Nov 8, 2017

TomDLT Nov 8, 2017 •

edited

Loading

rwolst commented Nov 8, 2017

lesteve commented Nov 8, 2017

jnothman left a comment

jnothman Nov 8, 2017

lesteve Nov 9, 2017

rwolst Jan 1, 2018

lesteve left a comment

lesteve Nov 9, 2017 •

edited

Loading

lesteve Nov 9, 2017

amueller Dec 15, 2017

rwolst Jan 1, 2018

jnothman commented Jan 1, 2018 via email

jnothman commented Jan 6, 2018

jnothman commented Aug 19, 2018

		return super(LogisticRegression, self)._predict_proba_lr(X)
		elif self.coef_.shape[0] == 1:

		@@ -565,6 +565,38 @@ def test_ovr_multinomial_iris():
		assert_equal(scores.shape, (3, n_cv, 10))


		def test_ovr_multinomial_iris_binary():

[MRG+1] Fix incorrect predict_proba for LogisticRegression in binary case using multinomial parameter. #9939

[MRG+1] Fix incorrect predict_proba for LogisticRegression in binary case using multinomial parameter. #9939

Conversation

rwolst commented Oct 17, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

TomDLT 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwolst commented Oct 26, 2017

lesteve commented Nov 8, 2017

TomDLT Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

rwolst commented Nov 8, 2017

lesteve commented Nov 8, 2017

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

lesteve left a comment

Choose a reason for hiding this comment

lesteve Nov 9, 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

jnothman commented Jan 1, 2018 via email

jnothman commented Jan 6, 2018

jnothman commented Aug 19, 2018

[MRG+1] Fix incorrect `predict_proba` for `LogisticRegression` in binary case using `multinomial` parameter. #9939

[MRG+1] Fix incorrect `predict_proba` for `LogisticRegression` in binary case using `multinomial` parameter. #9939

TomDLT Nov 8, 2017 •

edited

Loading

lesteve Nov 9, 2017 •

edited

Loading