Skip to content

[MRG+1] Add sample_weight support to RidgeClassifier #4838

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.

Sign up for GitHub

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
Jun 9, 2015
Merged

[MRG+1] Add sample_weight support to RidgeClassifier #4838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions doc/whats_new.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ Enhancements
It is now possible to ignore one or more labels, such as where
a multiclass problem has a majority class to ignore. By `Joel Nothman`_.

- Add ``sample_weight`` support to :class:`linear_model.RidgeClassifier`.
By `Trevor Stephens`_.

Bug fixes
.........
Expand Down
16 changes: 10 additions & 6 deletions sklearn/linear_model/ridge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,7 @@ def __init__(self, alpha=1.0, fit_intercept=True, normalize=False,
copy_X=copy_X, max_iter=max_iter, tol=tol, solver=solver)
self.class_weight = class_weight

def fit(self, X, y):
def fit(self, X, y, sample_weight=None):
"""Fit Ridge regression model.

Parameters
Expand All @@ -583,20 +583,24 @@ def fit(self, X, y):
y : array-like, shape = [n_samples]
Target values

sample_weight : float or numpy array of shape (n_samples,)
Sample weight.

Returns
-------
self : returns an instance of self.
"""
if sample_weight is None:
sample_weight = 1.
Copy link
Member

Choose a reason for hiding this comment

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

Could you also modify this line:
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/linear_model/ridge.py#L285
so that has_sw is False if sample_weight == 1.0.

sample_weight is currently implemented with a rescaling of the data. We can avoid copying the data in the case sample_weight == 1.0.

Another option is to check for sample_weight == 1.0 directly in _rescale_data.

Copy link
Member

Choose a reason for hiding this comment

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

sample_weight == 1.0 is semantically equivalent to sample_weight.ndim == 0?

On 10 June 2015 at 16:11, Mathieu Blondel notifications@github.com wrote:

In sklearn/linear_model/ridge.py
#4838 (comment)
:

     Returns
     -------
     self : returns an instance of self.
     """

  •    if sample_weight is None:

  •        sample_weight = 1.

Could you also modify this line:

https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/linear_model/ridge.py#L285
so that has_sw is False if sample_weight == 1.0.

sample_weight is currently implemented with a rescaling of the data. We
can avoid copying the data in the case sample_weight == 1.0.

Another option is to check for sample_weight == 1.0 directly in
_rescale_data.

—
Reply to this email directly or view it on GitHub
https://github.com/scikit-learn/scikit-learn/pull/4838/files#r32090535.

Copy link
Member

Choose a reason for hiding this comment

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

For scalar values other than 1, sample_weight has the same effect as changing C or 1/ alpha. So this is not very useful, we could potentially raise an exception.

Copy link
Member

Choose a reason for hiding this comment

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

I added #4846 to track this issue.


self._label_binarizer = LabelBinarizer(pos_label=1, neg_label=-1)
Y = self._label_binarizer.fit_transform(y)
if not self._label_binarizer.y_type_.startswith('multilabel'):
y = column_or_1d(y, warn=True)

if self.class_weight:
# get the class weight corresponding to each sample
sample_weight = compute_sample_weight(self.class_weight, y)
else:
sample_weight = None
# modify the sample weights with the corresponding class weight
sample_weight = (sample_weight *
compute_sample_weight(self.class_weight, y))

super(RidgeClassifier, self).fit(X, Y, sample_weight=sample_weight)
return self
Expand Down
29 changes: 29 additions & 0 deletions sklearn/linear_model/tests/test_ridge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,35 @@ def test_class_weights():
assert_array_almost_equal(clf.intercept_, clfa.intercept_)


def test_class_weight_vs_sample_weight():
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 a nice test, but is more appropriate as a common test than one specific to Ridge, and I think it's time for there to be common tests regarding sample weight. They would best rely on:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed @jnothman , nothing special to Ridge here, purely a test for classifiers with class_weight (constructor) and sample_weight (fit) to check equivalency and multiplicative combination. Should be able to work its way into a common test I would think. This test appears in the tree tests (though with a multi-output additional step_ and elsewhere (I think I may have pilfered or altered it from SGD originally).

Copy link
Member

Choose a reason for hiding this comment

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

I'd wondered whether it was copied from elsewhere. That's upsetting.
Perhaps we need an issue to track tests that should be factored into
common...

On 10 June 2015 at 14:28, Trevor Stephens notifications@github.com wrote:

In sklearn/linear_model/tests/test_ridge.py
#4838 (comment)
:

@@ -487,6 +487,35 @@ def test_class_weights():
assert_array_almost_equal(clf.intercept_, clfa.intercept_)

+def test_class_weight_vs_sample_weight():

Agreed @jnothman https://github.com/jnothman , nothing special to Ridge
here, purely a test for classifiers with class_weight (constructor) and
sample_weight (fit) to check equivalency and multiplicative combination.
Should be able to work its way into a common test I would think. This test
appears in the tree tests (though with a multi-output additional step_ and
elsewhere (I think I may have pilfered or altered it from SGD originally).

—
Reply to this email directly or view it on GitHub
https://github.com/scikit-learn/scikit-learn/pull/4838/files#r32087333.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upsetting how? That it's propagating outside of common tests?

Best I can trace right now was @dsullivan7 's #3931 (somewhat similar test on multiplicative weights), then RF & D-Tree in #3961 .. Now here in Ridge. Cannot recall now the exact lineage I used from the RF/tree PR tests. Don't see any duplicated inline comments outside of RF/Tree/Ridge on git FWIW.

Note that in tree-based classifier's I tested for equivalency in feature_importances_, and here it's coef_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something similar in #4215 coming soon enough too, I imagine, for the rest of the ensemble tests.

Copy link
Member

Choose a reason for hiding this comment

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

Note that in tree-based classifier's I tested for equivalency in
feature_importances_, and here it's coef_.

Hence the use of assert_same_model

On 10 June 2015 at 14:51, Joel Nothman joel.nothman@gmail.com wrote:

Upsetting that we're duplicating code and then going to need to rein it in.

On 10 June 2015 at 14:49, Trevor Stephens notifications@github.com
wrote:

In sklearn/linear_model/tests/test_ridge.py
#4838 (comment)
:

@@ -487,6 +487,35 @@ def test_class_weights():
assert_array_almost_equal(clf.intercept_, clfa.intercept_)

+def test_class_weight_vs_sample_weight():

Upsetting how? That it's propagating outside of common tests?

Best I can trace right now was @dsullivan7
https://github.com/dsullivan7 's #3931
#3931 (somewhat
similar test on multiplicative weights), then RF & D-Tree in #3961
#3961 .. Now here in
Ridge. Cannot recall now the exact lineage I used from the RF/tree PR
tests. Don't see any duplicated inline comments outside of RF/Tree/Ridge on
git FWIW.

Note that in tree-based classifier's I tested for equivalency in
feature_importances_, and here it's coef_.

—
Reply to this email directly or view it on GitHub
https://github.com/scikit-learn/scikit-learn/pull/4838/files#r32087957.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha

Copy link
Member

Choose a reason for hiding this comment

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

There are already common tests for class_weights, and special ones for linear classifiers: https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/estimator_checks.py#L1035 https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/estimator_checks.py#L1087

Copy link
Member

Choose a reason for hiding this comment

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

Adding one that checks if sample_weights is equivalent to doing the corresponding class_weights would be nice, too. I feel it should be pretty straight-forward, though. Why would you need a helper? Just check if decision_function is almost_equal.

Copy link
Member

Choose a reason for hiding this comment

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

True... as long as we don't have any classification-oriented transformers
that are not predictors that accept sample and class weights!

On 11 June 2015 at 03:46, Andreas Mueller notifications@github.com wrote:

In sklearn/linear_model/tests/test_ridge.py
#4838 (comment)
:

@@ -487,6 +487,35 @@ def test_class_weights():
assert_array_almost_equal(clf.intercept_, clfa.intercept_)

+def test_class_weight_vs_sample_weight():

Adding one that checks if sample_weights is equivalent to doing the
corresponding class_weights would be nice, too. I feel it should be
pretty straight-forward, though. Why would you need a helper? Just check if
decision_function is almost_equal.

—
Reply to this email directly or view it on GitHub
https://github.com/scikit-learn/scikit-learn/pull/4838/files#r32145069.

"""Check class_weights resemble sample_weights behavior."""
for clf in (RidgeClassifier, RidgeClassifierCV):

# Iris is balanced, so no effect expected for using 'balanced' weights
clf1 = clf()
clf1.fit(iris.data, iris.target)
clf2 = clf(class_weight='balanced')
clf2.fit(iris.data, iris.target)
assert_almost_equal(clf1.coef_, clf2.coef_)

# Inflate importance of class 1, check against user-defined weights
sample_weight = np.ones(iris.target.shape)
sample_weight[iris.target == 1] *= 100
class_weight = {0: 1., 1: 100., 2: 1.}
clf1 = clf()
clf1.fit(iris.data, iris.target, sample_weight)
clf2 = clf(class_weight=class_weight)
clf2.fit(iris.data, iris.target)
assert_almost_equal(clf1.coef_, clf2.coef_)

# Check that sample_weight and class_weight are multiplicative
clf1 = clf()
clf1.fit(iris.data, iris.target, sample_weight ** 2)
clf2 = clf(class_weight=class_weight)
clf2.fit(iris.data, iris.target, sample_weight)
assert_almost_equal(clf1.coef_, clf2.coef_)


def test_class_weights_cv():
# Test class weights for cross validated ridge classifier.
X = np.array([[-1.0, -1.0], [-1.0, 0], [-.8, -1.0],
Expand Down