Repository: scikit-learn/scikit-learn
Pull request: FIX raise error when enet_path and lasso_path receive unknown parameters
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/main/CONTRIBUTING.md
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Closes #10598
Takes over stalled PR and closes #10622


#### What does this implement/fix? Explain your changes.
- Raise ValueError when unknown params passed to enet_path
- Remove useless param in test_lasso_path_return_models_vs_new_return_gives_same_coefficients

#### Any other comments?


<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
enet_path does not pass params to coordinate descent solver
`params` parameter to `enet_path` is documented as "keyword arguments passed to the coordinate descent solver." Rather they are processed in `enet_path` and not directly passed anywhere. Thus currently one can pass arbitrary keyword arguments to `enet_path` without error (for instance I accidentally set `precomputed=True` instead of `precompute=True`).

We definitely need to raise an error when unknown params are passed in. We could also just expand the params in the function signature of `enet_path` (i.e. add X_offset, X_scale, tol, max_iter, random_state, selection) although that may be excessive for users.

Hints from issue discussion:
Can I take this? A tip to get me going would be nice though. Will try to finish within 1 week.
Basically we need to raise an error if excess parents are passed.

Hint: If instead of using params.get you use params.pop, any excess parameters will be easy to see.
Hello! @jnothman and @kkang2097 could I help with that?
Uh there's not much work to divide up, I mean it's a very quick 'try, except, raiseError'  few lines of code. If you are looking for issues to solve, look at 
projects >> sklearn 0.19 (priority listing for 0.19 release) and there should be hundreds of bugs that need solving.
OK! Tks!
So I'm proposing this:

```
func = sklearn.linear_model.coordinate_descent.enet_path
    for param in params:
        if param not in func.__code__.co_varnames:  
            raise ArgumentError
```

 `__code__.co_varnames` is a list that has the names of all the variables stored inside.
 The main idea is for the function to check if the argument in **params is acceptable,
 then raising an error if the argument is unrecognized.
 
**I don't think we should add a ton of extra arguments just for the sake of adding them. However, we should probably update the documentation on http://scikit-learn.org/stable/modules/generated/sklearn.linear_model.enet_path.html#sklearn.linear_model.enet_path to accommodate users that want to adjust things like random_state and the other unnamed parameters. I could do this also.
introspection like that should not be necessary...

Even if you just passed it with **params and let it check itself would be better
```
func = sklearn.linear_model.coordinate_descent.enet_path
        if self.params not in func.__code__.co_varnames:  
            raise ArgumentError
```
Huh?

Commit message: Flake8

Patch to review:
diff --git a/sklearn/linear_model/_coordinate_descent.py b/sklearn/linear_model/_coordinate_descent.py
index ea1216c0b55f0..ebdca9aa32ffe 100644
--- a/sklearn/linear_model/_coordinate_descent.py
+++ b/sklearn/linear_model/_coordinate_descent.py
@@ -435,6 +435,18 @@ def enet_path(X, y, *, l1_ratio=0.5, eps=1e-3, n_alphas=100, alphas=None,
     :ref:`examples/linear_model/plot_lasso_coordinate_descent_path.py
     <sphx_glr_auto_examples_linear_model_plot_lasso_coordinate_descent_path.py>`.
     """
+    X_offset_param = params.pop('X_offset', None)
+    X_scale_param = params.pop('X_scale', None)
+    tol = params.pop('tol', 1e-4)
+    max_iter = params.pop('max_iter', 1000)
+    random_state = params.pop('random_state', None)
+    selection = params.pop('selection', 'cyclic')
+
+    params.pop('fit_intercept', None)
+    params.pop('normalize', None)
+    if len(params) > 0:
+        raise ValueError("Unexpected parameters in params", params.keys())
+
     # We expect X and y to be already Fortran ordered when bypassing
     # checks
     if check_input:
@@ -460,10 +472,10 @@ def enet_path(X, y, *, l1_ratio=0.5, eps=1e-3, n_alphas=100, alphas=None,
 
     # MultiTaskElasticNet does not support sparse matrices
     if not multi_output and sparse.isspmatrix(X):
-        if 'X_offset' in params:
+        if X_offset_param is not None:
             # As sparse matrices are not actually centered we need this
             # to be passed to the CD solver.
-            X_sparse_scaling = params['X_offset'] / params['X_scale']
+            X_sparse_scaling = X_offset_param / X_scale_param
             X_sparse_scaling = np.asarray(X_sparse_scaling, dtype=X.dtype)
         else:
             X_sparse_scaling = np.zeros(n_features, dtype=X.dtype)
@@ -484,13 +496,10 @@ def enet_path(X, y, *, l1_ratio=0.5, eps=1e-3, n_alphas=100, alphas=None,
         alphas = np.sort(alphas)[::-1]  # make sure alphas are properly ordered
 
     n_alphas = len(alphas)
-    tol = params.get('tol', 1e-4)
-    max_iter = params.get('max_iter', 1000)
     dual_gaps = np.empty(n_alphas)
     n_iters = []
 
-    rng = check_random_state(params.get('random_state', None))
-    selection = params.get('selection', 'cyclic')
+    rng = check_random_state(random_state)
     if selection not in ['random', 'cyclic']:
         raise ValueError("selection should be either random or cyclic.")
     random = (selection == 'random')
diff --git a/sklearn/linear_model/tests/test_coordinate_descent.py b/sklearn/linear_model/tests/test_coordinate_descent.py
index 6e6f304bb9e24..0164ba08bde84 100644
--- a/sklearn/linear_model/tests/test_coordinate_descent.py
+++ b/sklearn/linear_model/tests/test_coordinate_descent.py
@@ -506,8 +506,7 @@ def test_lasso_path_return_models_vs_new_return_gives_same_coefficients():
     alphas_lars, _, coef_path_lars = lars_path(X, y, method='lasso')
     coef_path_cont_lars = interpolate.interp1d(alphas_lars[::-1],
                                                coef_path_lars[:, ::-1])
-    alphas_lasso2, coef_path_lasso2, _ = lasso_path(X, y, alphas=alphas,
-                                                    return_models=False)
+    alphas_lasso2, coef_path_lasso2, _ = lasso_path(X, y, alphas=alphas)
     coef_path_cont_lasso = interpolate.interp1d(alphas_lasso2[::-1],
                                                 coef_path_lasso2[:, ::-1])
 


Code review comments:
diff_hunk: @@ -435,6 +435,18 @@ def enet_path(X, y, *, l1_ratio=0.5, eps=1e-3, n_alphas=100, alphas=None,
     :ref:`examples/linear_model/plot_lasso_coordinate_descent_path.py
     <sphx_glr_auto_examples_linear_model_plot_lasso_coordinate_descent_path.py>`.
     """
+    X_offset_param = params.pop('X_offset', None)
+    X_scale_param = params.pop('X_scale', None)
+    tol = params.pop('tol', 1e-4)
+    max_iter = params.pop('max_iter', 1000)
+    random_state = params.pop('random_state', None)
+    selection = params.pop('selection', 'cyclic')
+
+    params.pop('fit_intercept', None)
+    params.pop('normalize', None)
path: sklearn/linear_model/_coordinate_descent.py
text: @author:
there seems to be many places in the tests where `fit_intercept` and and `normalize` are being passed into enet_path - changing `**params` to keyword args will necessitate a refactor of all tests that pass in fit_intercept and normalize. it seems that these two aren't being used in enet_path at all.

should we still proceed in this direction?

@user2:
I'd say yes, but I would also very much be interested in the opinion of others, @user1 maybe?

@user1:
Indeed, `fit_intercept` and `normalize` are not used in the path. They used to prepare the data before `fit`.
I assume that we should raise the error if one tries to pass these parameters since the `path` is not doing anything regarding these parameters.

However, we should add in the documentation that `lasso_path` and `lasso_path` expect centred and normalized data. If this is not the case, I don't know if we should stipulate to use the `Lasso` and `ElasticNet` estimators instead. @user3 can give some highlight here.