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] Add jitter to LassoLars #15179

Merged
merged 24 commits into from Apr 17, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 35 additions & 4 deletions sklearn/linear_model/_least_angle.py
Expand Up @@ -19,7 +19,7 @@

from ._base import LinearModel
from ..base import RegressorMixin, MultiOutputMixin
from ..utils import arrayfuncs, as_float_array, check_X_y
from ..utils import arrayfuncs, as_float_array, check_X_y, check_random_state
from ..model_selection import check_cv
from ..exceptions import ConvergenceWarning

Expand Down Expand Up @@ -799,6 +799,15 @@ class Lars(MultiOutputMixin, RegressorMixin, LinearModel):
setting ``fit_path`` to ``False`` will lead to a speedup, especially
with a small alpha.

jitter : float, default=None
Uniform noise parameter, added to the y values, to satisfy
the model's assumption of one-at-a-time computations.
NicolasHug marked this conversation as resolved.
Show resolved Hide resolved

random_state : int, RandomState instance or None (default)
Determines random number generation for dataset creation. Pass an int
for reproducible output across multiple function calls.
See :term:`Glossary <random_state>`.
NicolasHug marked this conversation as resolved.
Show resolved Hide resolved

Attributes
----------
alphas_ : array-like of shape (n_alphas + 1,) | list of n_targets such \
Expand Down Expand Up @@ -845,7 +854,8 @@ class Lars(MultiOutputMixin, RegressorMixin, LinearModel):

def __init__(self, fit_intercept=True, verbose=False, normalize=True,
precompute='auto', n_nonzero_coefs=500,
eps=np.finfo(np.float).eps, copy_X=True, fit_path=True):
eps=np.finfo(np.float).eps, copy_X=True, fit_path=True,
jitter=None, random_state=None):
self.fit_intercept = fit_intercept
self.verbose = verbose
self.normalize = normalize
Expand All @@ -854,6 +864,8 @@ def __init__(self, fit_intercept=True, verbose=False, normalize=True,
self.eps = eps
self.copy_X = copy_X
self.fit_path = fit_path
self.jitter = jitter
self.random_state = random_state

@staticmethod
def _get_gram(precompute, X, y):
Expand Down Expand Up @@ -953,6 +965,12 @@ def fit(self, X, y, Xy=None):
else:
max_iter = self.max_iter

if self.jitter is not None:
rng = check_random_state(self.random_state)

noise = rng.uniform(high=self.jitter, size=len(y))
y = y + noise

self._fit(X, y, max_iter=max_iter, alpha=alpha, fit_path=self.fit_path,
Xy=Xy)

Expand Down Expand Up @@ -1030,6 +1048,15 @@ class LassoLars(Lars):
algorithm are typically in congruence with the solution of the
coordinate descent Lasso estimator.

jitter : float, default=None
Uniform noise parameter, added to the y values, to satisfy
the model's assumption of one-at-a-time computations.
NicolasHug marked this conversation as resolved.
Show resolved Hide resolved

random_state : int, RandomState instance or None (default)
Determines random number generation for dataset creation. Pass an int
for reproducible output across multiple function calls.
See :term:`Glossary <random_state>`.
NicolasHug marked this conversation as resolved.
Show resolved Hide resolved

Attributes
----------
alphas_ : array-like of shape (n_alphas + 1,) | list of n_targets such \
Expand Down Expand Up @@ -1082,7 +1109,7 @@ class LassoLars(Lars):
def __init__(self, alpha=1.0, fit_intercept=True, verbose=False,
normalize=True, precompute='auto', max_iter=500,
eps=np.finfo(np.float).eps, copy_X=True, fit_path=True,
positive=False):
positive=False, jitter=None, random_state=None):
self.alpha = alpha
self.fit_intercept = fit_intercept
self.max_iter = max_iter
Expand All @@ -1093,6 +1120,8 @@ def __init__(self, alpha=1.0, fit_intercept=True, verbose=False,
self.copy_X = copy_X
self.eps = eps
self.fit_path = fit_path
self.jitter = jitter
self.random_state = random_state


###############################################################################
Expand Down Expand Up @@ -1710,7 +1739,8 @@ class LassoLarsIC(LassoLars):
"""
def __init__(self, criterion='aic', fit_intercept=True, verbose=False,
normalize=True, precompute='auto', max_iter=500,
eps=np.finfo(np.float).eps, copy_X=True, positive=False):
eps=np.finfo(np.float).eps, copy_X=True, positive=False,
random_state=None):
Copy link
Member

Choose a reason for hiding this comment

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

why adding a random_state param here and not jitter?

self.criterion = criterion
self.fit_intercept = fit_intercept
self.positive = positive
Expand All @@ -1721,6 +1751,7 @@ def __init__(self, criterion='aic', fit_intercept=True, verbose=False,
self.precompute = precompute
self.eps = eps
self.fit_path = True
self.random_state = random_state

def _more_tags(self):
return {'multioutput': False}
Expand Down
33 changes: 33 additions & 0 deletions sklearn/linear_model/tests/test_least_angle.py
Expand Up @@ -733,6 +733,39 @@ def test_lasso_lars_fit_copyX_behaviour(copy_X):
assert copy_X == np.array_equal(X, X_copy)


@pytest.mark.parametrize('y_list, expected_y', [
NicolasHug marked this conversation as resolved.
Show resolved Hide resolved
([-2.5, -2.5], [0, 2.5, 0, 2.5, 0]),
([[-2.5, -2.5], [-2.5, -2.5]],
[[0, 5, 0, 2.5, 0], [0, 5, 0, 2.5, 0]])])
def test_lars_with_jitter(y_list, expected_y):
"""
Test that user input of a small amount of jitter,
using example provided in issue #2746

"""
NicolasHug marked this conversation as resolved.
Show resolved Hide resolved

X = np.array([[0.0, 0.0, 0.0, -1.0, 0.0], [0.0, -1.0, 0.0, 0.0, 0.0]])
y = np.array(y_list)
expected_output = np.array(expected_y)
NicolasHug marked this conversation as resolved.
Show resolved Hide resolved
alpha = 0.001
fit_intercept = False
Copy link
Member

Choose a reason for hiding this comment

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

why forcing fit_intercept = False?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the at the edge of my stats/linear algebra understanding, but I think we need to force it to be False since the error only occurs for exactly aligned values (e.g. this comment).

Copy link
Member

Choose a reason for hiding this comment

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

from your comment I understand that the test would not be a non-regression test with fit_intercept=True as you only see an error with fit_intercept. However below the X and y you chose do work too with jitter=None. Did you check that the test still pass with fit_intercept=True?

Copy link
Member

Choose a reason for hiding this comment

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

Since the target is constant (-2.5, -2.5), fitting with the intercept would mean the coeffs are just all 0 (and the intercept is -2.5).

So I guess it makes sense to leave fit_intercept=False. It properly reproduces the original example from the issue


lars = linear_model.LassoLars(alpha=alpha, fit_intercept=fit_intercept)
lars_with_jitter = linear_model.LassoLars(alpha=alpha,
fit_intercept=fit_intercept,
jitter=10e-8,
random_state=0)

lars.fit(X, y)
lars_with_jitter.fit(X, y)

w_nojitter = lars.coef_
w_jitter = lars_with_jitter.coef_

assert not np.array_equal(w_jitter, w_nojitter)
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 too easy to pass, so maybe instead check the MSD

assert np.mean((w_jitter - w_nojitter)**2) > .1

assert_array_almost_equal(w_jitter, expected_output, decimal=2)
NicolasHug marked this conversation as resolved.
Show resolved Hide resolved


def test_X_none_gram_not_none():
with pytest.raises(ValueError,
match="X cannot be None if Gram is not None"):
Expand Down