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+1] Fix incorrect `predict_proba` for `LogisticRegression` in binary case using `multinomial` parameter. #9939

Merged
merged 9 commits into from Jan 6, 2018

Conversation

Projects
None yet
5 participants
@rwolst
Contributor

rwolst commented Oct 17, 2017

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?

@TomDLT

TomDLT approved these changes Oct 18, 2017

LGTM, only nitpicks

Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
Show outdated Hide outdated sklearn/linear_model/logistic.py Outdated
Show outdated Hide outdated sklearn/linear_model/logistic.py Outdated

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

@rwolst

This comment has been minimized.

Show comment
Hide comment
@rwolst

rwolst Oct 26, 2017

Contributor

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

Contributor

rwolst commented Oct 26, 2017

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

Merge branch 'master' into lr_multinomial_bug_fix
On top of the merge, added the changes suggested in pull request.
@lesteve

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Nov 8, 2017

Member

Can you add a non-regression test please?

Member

lesteve commented Nov 8, 2017

Can you add a non-regression test please?

Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
@rwolst

This comment has been minimized.

Show comment
Hide comment
@rwolst

rwolst Nov 8, 2017

Contributor

@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.

Contributor

rwolst commented Nov 8, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@lesteve

lesteve Nov 8, 2017

Member

@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.

Member

lesteve commented Nov 8, 2017

@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

Show outdated Hide outdated doc/whats_new/v0.20.rst Outdated
@lesteve

Some small comments

Show outdated Hide outdated sklearn/linear_model/logistic.py Outdated
Show outdated Hide outdated sklearn/linear_model/tests/test_logistic.py Outdated
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jan 1, 2018

Member
Member

jnothman commented Jan 1, 2018

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jan 6, 2018

Member

Thanks a lot, @rwolst. Merging!

Member

jnothman commented Jan 6, 2018

Thanks a lot, @rwolst. Merging!

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

6 checks passed

ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: python2 Your tests passed on CircleCI!
Details
ci/circleci: python3 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 19, 2018

Member

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.

Member

jnothman commented Aug 19, 2018

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.

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