Skip to content

[MRG+1] use class_weight through sample_weight in LogisticRegression with liblinear #6817

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

Merged
merged 1 commit into from
May 25, 2016

Conversation

TomDLT
Copy link
Member

@TomDLT TomDLT commented May 24, 2016

I forgot to update #5008 after we merged #5274 (cf. #5008 (comment)).

It makes the class_weight behavior consistent with all solver of LogisticRegression, by putting the class_weight into the sample_weight.

Fixes #6795

"solver cannot handle multiclass with "
"class_weight of type dict. Use the lbfgs, "
"newton-cg or sag solvers or set "
"class_weight='balanced'")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not true anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, since liblinear solver handles sample_weight (#5274), we can make the behavior identical for all LogisticRegression's solvers, by merging the class_weight into the sample_weight.

@agramfort agramfort changed the title [MRG] use class_weight through sample_weight in LogisticRegression with liblinear [MRG+1] use class_weight through sample_weight in LogisticRegression with liblinear May 25, 2016
@agramfort
Copy link
Member

+1 for merge

for class_weight in ({0: 0.1, 1: 0.2, 2: 0.5}, 'balanced'):
for n_classes in [3, 2]:
if n_classes == 2 and class_weight != 'balanced':
class_weight.pop(2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid bugs if this were extended, can we please do class_weight = class_weight.copy() first?

Copy link
Member Author

@TomDLT TomDLT May 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the loops to avoid modifications on class_weight.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the loops to avoid modifications on class_weight.

Yes, of course that's the sensible way to do it!

@jnothman
Copy link
Member

Otherwise, LGTM

@TomDLT TomDLT force-pushed the logistic_class_weight branch from 2c33e39 to c640fce Compare May 25, 2016 11:47
@jnothman
Copy link
Member

would you like to add to whats_new, then squash?

@TomDLT TomDLT force-pushed the logistic_class_weight branch from c640fce to 1107f22 Compare May 25, 2016 12:54
@TomDLT
Copy link
Member Author

TomDLT commented May 25, 2016

Done. I also added ConvergenceWarning in newton-cg solver.

@jnothman
Copy link
Member

Let's do it.

@jnothman jnothman merged commit af171b8 into scikit-learn:master May 25, 2016
@TomDLT TomDLT deleted the logistic_class_weight branch December 20, 2016 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants