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+1] Fixes #10393 Fixed error when fitting RidgeCV with integers #10397

Merged
5 changes: 4 additions & 1 deletion doc/whats_new/v0.20.rst
Expand Up @@ -277,6 +277,9 @@ Classifiers and regressors
where the coefficient had wrong shape when ``fit_intercept=False``.
:issue:`10687` by :user:`Martin Hahn <martin-hahn>`.

- Fixed a bug in :class:`linear_model.RidgeCV` where using integer ``alphas``
raised an error. :issue:`10393` by :user:`Mabel Villalba-Jiménez <mabelvj>`.

Decomposition, manifold learning and clustering

- Fix for uninformative error in :class:`decomposition.IncrementalPCA`:
Expand Down Expand Up @@ -471,4 +474,4 @@ Changes to estimator checks

- Add test :func:`estimator_checks.check_methods_subset_invariance` to check
that estimators methods are invariant if applied to a data subset.
:issue:`10420` by :user:`Jonathan Ohayon <Johayon>`
Copy link
Member

Choose a reason for hiding this comment

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

Try to get rid of this strange diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, the file in the master has already that line. Should I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

If you actually don't change anything and find it hard to get rid of it, you might just keep it. (Hope there won't be some strange things when merging)

:issue:`10420` by :user:`Jonathan Ohayon <Johayon>`
13 changes: 10 additions & 3 deletions sklearn/linear_model/ridge.py
Expand Up @@ -778,6 +778,7 @@ class RidgeClassifier(LinearClassifierMixin, _BaseRidge):
a one-versus-all approach. Concretely, this is implemented by taking
advantage of the multi-variate response support in Ridge.
"""

def __init__(self, alpha=1.0, fit_intercept=True, normalize=False,
copy_X=True, max_iter=None, tol=1e-3, class_weight=None,
solver="auto", random_state=None):
Expand Down Expand Up @@ -1041,11 +1042,16 @@ def fit(self, X, y, sample_weight=None):
scorer = check_scoring(self, scoring=self.scoring, allow_none=True)
error = scorer is None

if np.any(self.alphas < 0):
raise ValueError("alphas cannot be negative. "
"Got {} containing some "
"negative value instead.".format(self.alphas))

for i, alpha in enumerate(self.alphas):
if error:
out, c = _errors(alpha, y, v, Q, QT_y)
out, c = _errors(float(alpha), y, v, Q, QT_y)
else:
out, c = _values(alpha, y, v, Q, QT_y)
out, c = _values(float(alpha), y, v, Q, QT_y)
cv_values[:, i] = out.ravel()
C.append(c)

Expand Down Expand Up @@ -1085,7 +1091,7 @@ def __init__(self, alphas=(0.1, 1.0, 10.0),
fit_intercept=True, normalize=False, scoring=None,
cv=None, gcv_mode=None,
store_cv_values=False):
self.alphas = alphas
self.alphas = np.asarray(alphas)
Copy link
Member

Choose a reason for hiding this comment

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

Why doing so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was suggested to make that changes a few lines above: to add a conversion in the __init__ and then remove the float. It's done in the init of _RidgeGCV.

self.fit_intercept = fit_intercept
self.normalize = normalize
self.scoring = scoring
Expand Down Expand Up @@ -1328,6 +1334,7 @@ class RidgeClassifierCV(LinearClassifierMixin, _BaseRidgeCV):
a one-versus-all approach. Concretely, this is implemented by taking
advantage of the multi-variate response support in Ridge.
"""

def __init__(self, alphas=(0.1, 1.0, 10.0), fit_intercept=True,
normalize=False, scoring=None, cv=None, class_weight=None):
super(RidgeClassifierCV, self).__init__(
Expand Down
30 changes: 30 additions & 0 deletions sklearn/linear_model/tests/test_ridge.py
Expand Up @@ -11,6 +11,7 @@
from sklearn.utils.testing import assert_greater
from sklearn.utils.testing import assert_raises
from sklearn.utils.testing import assert_raise_message
from sklearn.utils.testing import assert_raises_regex
from sklearn.utils.testing import ignore_warnings
from sklearn.utils.testing import assert_warns

Expand Down Expand Up @@ -51,6 +52,7 @@
X_iris = sp.csr_matrix(iris.data)
y_iris = iris.target


DENSE_FILTER = lambda X: X
SPARSE_FILTER = lambda X: sp.csr_matrix(X)

Expand Down Expand Up @@ -704,6 +706,34 @@ def test_sparse_design_with_sample_weights():
decimal=6)


def test_ridgecv_int_alphas():
X = np.array([[-1.0, -1.0], [-1.0, 0], [-.8, -1.0],
[1.0, 1.0], [1.0, 0.0]])
y = [1, 1, 1, -1, -1]

# Integers
ridge = RidgeCV(alphas=(1, 10, 100))
ridge.fit(X, y)


def test_ridgecv_negative_alphas():
X = np.array([[-1.0, -1.0], [-1.0, 0], [-.8, -1.0],
[1.0, 1.0], [1.0, 0.0]])
y = [1, 1, 1, -1, -1]

# Negative integers
ridge = RidgeCV(alphas=(-1, -10, -100))
assert_raises_regex(ValueError,
"alphas cannot be negative.",
ridge.fit, X, y)

# Negative floats
ridge = RidgeCV(alphas=(-0.1, -1.0, -10.0))
assert_raises_regex(ValueError,
"alphas cannot be negative.",
ridge.fit, X, y)


def test_raises_value_error_if_solver_not_supported():
# Tests whether a ValueError is raised if a non-identified solver
# is passed to ridge_regression
Expand Down