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] ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' #11905

Merged
merged 25 commits into from Aug 26, 2018

Conversation

Projects
None yet
3 participants
@jnothman
Member

jnothman commented Aug 24, 2018

Adds multi_class='auto'

        If the option chosen is 'ovr', then a binary problem is fit for each
        label. For 'multinomial' the loss minimised is the multinomial loss fit
        across the entire probability distribution, *even when the data is
        binary*. 'multinomial' is unavailable when solver='liblinear'.
        'auto' selects 'ovr' if the data is binary, or if solver='liblinear',
        and otherwise selects 'multinomial'.

'auto' will be default from 0.22. Also sets solver='lbfgs' to default from 0.22.

This is okay to merge for RC, but I've not yet done a couple of things we should do for the final version:

  • don't warn when multi_class='warn' but y is binary
  • don't explicitly specify multi_class in docs/examples/tests when warning will therefore be silenced.

I may not have time to finish this for a couple of days.

Supersedes and closes #11476
Supersedes and closes #10001
Closes #9997

TomDLT and others added some commits Jul 6, 2018

@jnothman jnothman changed the title from ENH add multi_class='auto' for LogisticRegression, default from 0.22 to ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' Aug 24, 2018

jnothman added some commits Aug 24, 2018

@rth

rth approved these changes Aug 24, 2018

LGTM. I think the multi_class= 'auto' is indeed better than the earlier alternatives.

+1 for merging and fixing the remaining points mentioned in the description after the RC.

@amueller or @agramfort would you be able to have a look?

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

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 25, 2018

Member

If you've verified that you're happy with the new tests, I think we can just merge and get the release on its way.

Member

jnothman commented Aug 25, 2018

If you've verified that you're happy with the new tests, I think we can just merge and get the release on its way.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 25, 2018

Member

btw, I did change it to not warn upon binary, but I've not fixed up the examples, docs, tests to not pass redundantly.

Member

jnothman commented Aug 25, 2018

btw, I did change it to not warn upon binary, but I've not fixed up the examples, docs, tests to not pass redundantly.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Aug 25, 2018

Member

Oh, this didn't change any examples, so...

Member

jnothman commented Aug 25, 2018

Oh, this didn't change any examples, so...

@jnothman

Ready for merge.

@jnothman jnothman changed the title from ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' to [MRG+1] ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' Aug 25, 2018

y_bin = y_multi == 0
est_auto_bin = fit(X, y_bin, multi_class='auto', solver=solver)
est_ovr_bin = fit(X, y_bin, multi_class='ovr', solver=solver)
assert np.allclose(est_auto_bin.coef_, est_ovr_bin.coef_)

This comment has been minimized.

@rth

rth Aug 26, 2018

Member

Not critical, but I think assert_allclose produces better trackbacks in the case of failure, also it has a mores strict default relative and absolute tolerances we might want to use one or the other but not mix both.

@rth

rth Aug 26, 2018

Member

Not critical, but I think assert_allclose produces better trackbacks in the case of failure, also it has a mores strict default relative and absolute tolerances we might want to use one or the other but not mix both.

solver=solver).coef_)
assert not np.allclose(est_auto_bin.coef_,
fit(X, y_multi, multi_class='multinomial',
solver=solver).coef_)

This comment has been minimized.

@rth

rth Aug 26, 2018

Member

Overall I think these tests are fairly thorough!

@rth

rth Aug 26, 2018

Member

Overall I think these tests are fairly thorough!

@rth

This comment has been minimized.

Show comment
Hide comment
@rth

rth Aug 26, 2018

Member

Merging with a +2 in the parent PR (#11476) and another +1 from #11476 (comment) for the added changes.

Thanks a lot @jnothman for all your work on making the RC happen!

Member

rth commented Aug 26, 2018

Merging with a +2 in the parent PR (#11476) and another +1 from #11476 (comment) for the added changes.

Thanks a lot @jnothman for all your work on making the RC happen!

@rth rth merged commit 7ed61a2 into scikit-learn:master Aug 26, 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 99.17% of diff hit (target 92.89%)
Details
codecov/project 92.9% (+0.01%) compared to 2fe58e5
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Sep 10, 2018

Merge tag '0.20rc1' into releases
* tag '0.20rc1': (1109 commits)
  MNT rc version
  DOC Release dates for 0.20 (scikit-learn#11838)
  DOC Fix: require n_splits > 1 in TimeSeriesSplit (scikit-learn#11937)
  FIX xfail for MacOS LogisticRegressionCV stability (scikit-learn#11936)
  MNT: Use GEMV in enet_coordinate_descent (Pt. 1) (scikit-learn#11896)
  [MRG] TST/FIX stop optics reachability failure on 32bit (scikit-learn#11916)
  ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' (scikit-learn#11905)
  MAINT Fix test_logistic::test_dtype_match failure on 32 bit arch (scikit-learn#11899)
  DOC Updated link to Laurens van der Maaten's home page (scikit-learn#11907)
  DOC Remove stray backtick in /doc/modules/feature_extraction.rst (scikit-learn#11910)
  Deprecate min_samples_leaf and min_weight_fraction_leaf (scikit-learn#11870)
  MNT modify test_sparse_oneclasssvm to be parametrized (scikit-learn#11894)
  EXA set figure size to avoid overlaps (scikit-learn#11889)
  MRG/REL fixes /skips for 32bit tests (scikit-learn#11879)
  add durations=20 to makefile to show test runtimes locally (scikit-learn#11147)
  DOC loss='l2' is no longer accpeted in l1_min_c
  DOC add note about brute force nearest neighbors for string data (scikit-learn#11884)
  DOC Change sign of energy in RBM (scikit-learn#11156)
  RFC try to warn on iid less often (scikit-learn#11613)
  DOC reduce plot_gpr_prior_posterior.py warnings(scikit-learn#11664)
  ...

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Sep 10, 2018

Merge branch 'releases' into dfsg
* releases: (1109 commits)
  MNT rc version
  DOC Release dates for 0.20 (scikit-learn#11838)
  DOC Fix: require n_splits > 1 in TimeSeriesSplit (scikit-learn#11937)
  FIX xfail for MacOS LogisticRegressionCV stability (scikit-learn#11936)
  MNT: Use GEMV in enet_coordinate_descent (Pt. 1) (scikit-learn#11896)
  [MRG] TST/FIX stop optics reachability failure on 32bit (scikit-learn#11916)
  ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' (scikit-learn#11905)
  MAINT Fix test_logistic::test_dtype_match failure on 32 bit arch (scikit-learn#11899)
  DOC Updated link to Laurens van der Maaten's home page (scikit-learn#11907)
  DOC Remove stray backtick in /doc/modules/feature_extraction.rst (scikit-learn#11910)
  Deprecate min_samples_leaf and min_weight_fraction_leaf (scikit-learn#11870)
  MNT modify test_sparse_oneclasssvm to be parametrized (scikit-learn#11894)
  EXA set figure size to avoid overlaps (scikit-learn#11889)
  MRG/REL fixes /skips for 32bit tests (scikit-learn#11879)
  add durations=20 to makefile to show test runtimes locally (scikit-learn#11147)
  DOC loss='l2' is no longer accpeted in l1_min_c
  DOC add note about brute force nearest neighbors for string data (scikit-learn#11884)
  DOC Change sign of energy in RBM (scikit-learn#11156)
  RFC try to warn on iid less often (scikit-learn#11613)
  DOC reduce plot_gpr_prior_posterior.py warnings(scikit-learn#11664)
  ...

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Sep 10, 2018

Merge branch 'dfsg' into debian
* dfsg: (1109 commits)
  MNT rc version
  DOC Release dates for 0.20 (scikit-learn#11838)
  DOC Fix: require n_splits > 1 in TimeSeriesSplit (scikit-learn#11937)
  FIX xfail for MacOS LogisticRegressionCV stability (scikit-learn#11936)
  MNT: Use GEMV in enet_coordinate_descent (Pt. 1) (scikit-learn#11896)
  [MRG] TST/FIX stop optics reachability failure on 32bit (scikit-learn#11916)
  ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' (scikit-learn#11905)
  MAINT Fix test_logistic::test_dtype_match failure on 32 bit arch (scikit-learn#11899)
  DOC Updated link to Laurens van der Maaten's home page (scikit-learn#11907)
  DOC Remove stray backtick in /doc/modules/feature_extraction.rst (scikit-learn#11910)
  Deprecate min_samples_leaf and min_weight_fraction_leaf (scikit-learn#11870)
  MNT modify test_sparse_oneclasssvm to be parametrized (scikit-learn#11894)
  EXA set figure size to avoid overlaps (scikit-learn#11889)
  MRG/REL fixes /skips for 32bit tests (scikit-learn#11879)
  add durations=20 to makefile to show test runtimes locally (scikit-learn#11147)
  DOC loss='l2' is no longer accpeted in l1_min_c
  DOC add note about brute force nearest neighbors for string data (scikit-learn#11884)
  DOC Change sign of energy in RBM (scikit-learn#11156)
  RFC try to warn on iid less often (scikit-learn#11613)
  DOC reduce plot_gpr_prior_posterior.py warnings(scikit-learn#11664)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment