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+2] Fix SGD partial_fit multiclass w/ average. #5282

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@andylamb
Contributor

andylamb commented Sep 16, 2015

See #5246 (comment).

_fit_multiclass was setting self.intercept_ to a single element of the intercept, instead of the entire intercept.

Fix SGD partial_fit multiclass w/ average.
See #5246 (comment).

`_fit_multiclass` was setting `self.intercept_` to a single element of the intercept, instead of the entire intercept.
@TomDLT

This comment has been minimized.

Member

TomDLT commented Sep 18, 2015

LGTM
Added test fails on master, as expected

@GaelVaroquaux GaelVaroquaux changed the title from Fix SGD partial_fit multiclass w/ average. to [MRG+2] Fix SGD partial_fit multiclass w/ average. Sep 18, 2015

@GaelVaroquaux

This comment has been minimized.

Member

GaelVaroquaux commented Sep 18, 2015

LGTM. This is ready to merge, but ideally an entry in docs/whats_new.rst would be great.

@andylamb

This comment has been minimized.

Contributor

andylamb commented Sep 18, 2015

Added an entry to whats_new.rst.

@ogrisel ogrisel added this to the 0.17 milestone Oct 7, 2015

@ogrisel

This comment has been minimized.

Member

ogrisel commented Oct 7, 2015

This code could probably be improved to be more readable and better documented (the standard_{coef, intercept}_ and average_{coef, intercept}_ attributes are not documented at all in the docstring. I am not sure they should be public attributes in the first place.

Anyway the fix looks correct. I will merge it by rebase.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Oct 7, 2015

Done in 176427e. Thanks for the fix @andylamb!

@ogrisel ogrisel closed this Oct 7, 2015

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