Skip to content
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] Fixed bug in _log_reg_scoring_path for multinomial case #11724

Merged
merged 4 commits into from Aug 2, 2018

Conversation

Projects
None yet
3 participants
@NicolasHug
Copy link
Contributor

NicolasHug commented Aug 1, 2018

Reference Issues/PRs

What does this implement/fix? Explain your changes.

This PR fixes a weird bug in logistic_regression_path that makes _log_reg_scoring_path give an incorrect output. Prior to this change, all the scores (from the different C values) would have been the same.

Any other comments?

I stumbled upon this while working on #11646.

To be fair I suspect that there are other bugs.

Related but not part of this PR: the doc of logistic_regression_path says that the coefs attribute is of shape (n_cs, n_features) but that's not true when multiclass=multinomial. Should I update the doc? Or is the actual code that is wrong?

NicolasHug added some commits Aug 1, 2018

@jnothman
Copy link
Member

jnothman left a comment

Yikes! This is a nasty one... Well spotted. Add a what's new entry, please



def test_log_reg_scoring_path_multinomial():
# Make sure that the scores are all different for different C values

This comment has been minimized.

@jnothman

jnothman Aug 1, 2018

Member

At the moment you are only checking that some are different. And if so, you only need to test two values of C

NicolasHug added some commits Aug 1, 2018

Modified test so that it better checks the new changes
Also added entry in whats new and fixed doc
@TomDLT

TomDLT approved these changes Aug 2, 2018

@TomDLT

This comment has been minimized.

Copy link
Member

TomDLT commented Aug 2, 2018

Good catch, thanks !

@TomDLT TomDLT merged commit e500447 into scikit-learn:master Aug 2, 2018

7 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
codecov/patch 100% of diff hit (target 95.3%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +4.69% compared to b21f9b4
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@NicolasHug NicolasHug deleted the NicolasHug:fix_scoring_path branch Aug 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.