Repository: scikit-learn/scikit-learn
Pull request: Dictionary Learning: transform_alpha default equal to alpha
…ctionaryLearning

<!--
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/master/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
-->

Fixes #19154 

#### What does this implement/fix? Explain your changes.

Setting the default value of transform_alpha to alpha in order to avoid confusion. 

#### 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:
Dictionary Learning: transform_alpha default is not equal to alpha ?
Hello, 

I recently noticed that the parameter `transform_alpha` in `sklearn.decomposition.DictionaryLearning` has a default value equal to 1.0 and not equal to `alpha` (the main regularization parameter). I think it can be misleading, because we expect the sparse code computed by `transform` to correspond to the regularization parameter that was used to learn the dictionary, and not some random float. 

Is there any reason why it is done that way ? If so, maybe it could be made clearer in the doc.

Thank you in advance for your help !

Hints from issue discussion:
I agree it's a bit confusing. However the default transform algorithm ('omp') is different from the default fit_algorithm ('lars' but it's actually lasso_lars) and the regularization is not the same. For 'omp' it's controlled by ``transform_n_nonzero_coefs``.

The doc could definitely be clearer. Would you be interested in submitting a PR ?
Maybe it would still make sense that in case `transform_algorithm` is the same as `fit_algorithm` and `transform_alpha` is None, you use `alpha` no?

I agree that clarifying would help a lot.
> Maybe it would still make sense that in case `transform_algorithm` is the same as `fit_algorithm` and `transform_alpha` is None, you use `alpha` no?
> 
> I agree that clarifying would help a lot.

I agree, and I am interested in submitting a PR

Commit message: Update sklearn/decomposition/_dict_learning.py

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>

Patch to review:
diff --git a/sklearn/decomposition/_dict_learning.py b/sklearn/decomposition/_dict_learning.py
index 781f288b70351..fe139a4cf6114 100644
--- a/sklearn/decomposition/_dict_learning.py
+++ b/sklearn/decomposition/_dict_learning.py
@@ -909,6 +909,9 @@ def _transform(self, X, dictionary):
         SparseCoder."""
         X = self._validate_data(X, reset=False)
 
+        if hasattr(self, "alpha") and self.transform_alpha is None:
+            self.transform_alpha = self.alpha
+
         code = sparse_encode(
             X, dictionary, algorithm=self.transform_algorithm,
             n_nonzero_coefs=self.transform_n_nonzero_coefs,
@@ -1184,19 +1187,15 @@ class DictionaryLearning(_BaseSparseCoding, BaseEstimator):
 
     transform_n_nonzero_coefs : int, default=None
         Number of nonzero coefficients to target in each column of the
-        solution. This is only used by `algorithm='lars'` and `algorithm='omp'`
-        and is overridden by `alpha` in the `omp` case. If `None`, then
-        `transform_n_nonzero_coefs=int(n_features / 10)`.
+        solution. This is only used by `algorithm='lars'` and `algorithm='omp'`.
+        If `None`, then `transform_n_nonzero_coefs=int(n_features / 10)`.
 
     transform_alpha : float, default=None
         If `algorithm='lasso_lars'` or `algorithm='lasso_cd'`, `alpha` is the
         penalty applied to the L1 norm.
         If `algorithm='threshold'`, `alpha` is the absolute value of the
         threshold below which coefficients will be squashed to zero.
-        If `algorithm='omp'`, `alpha` is the tolerance parameter: the value of
-        the reconstruction error targeted. In this case, it overrides
-        `n_nonzero_coefs`.
-        If `None`, default to 1.0
+        If `None`, defaults to `alpha`.
 
     n_jobs : int or None, default=None
         Number of parallel jobs to run.
@@ -1424,19 +1423,15 @@ class MiniBatchDictionaryLearning(_BaseSparseCoding, BaseEstimator):
 
     transform_n_nonzero_coefs : int, default=None
         Number of nonzero coefficients to target in each column of the
-        solution. This is only used by `algorithm='lars'` and `algorithm='omp'`
-        and is overridden by `alpha` in the `omp` case. If `None`, then
-        `transform_n_nonzero_coefs=int(n_features / 10)`.
+        solution. This is only used by `algorithm='lars'` and `algorithm='omp'`.
+        If `None`, then `transform_n_nonzero_coefs=int(n_features / 10)`.
 
     transform_alpha : float, default=None
         If `algorithm='lasso_lars'` or `algorithm='lasso_cd'`, `alpha` is the
         penalty applied to the L1 norm.
         If `algorithm='threshold'`, `alpha` is the absolute value of the
         threshold below which coefficients will be squashed to zero.
-        If `algorithm='omp'`, `alpha` is the tolerance parameter: the value of
-        the reconstruction error targeted. In this case, it overrides
-        `n_nonzero_coefs`.
-        If `None`, default to 1.
+        If `None`, defaults to `alpha`.
 
     verbose : bool, default=False
         To control the verbosity of the procedure.


Code review comments:
diff_hunk: @@ -909,6 +909,9 @@ def _transform(self, X, dictionary):
         SparseCoder."""
         X = self._validate_data(X, reset=False)
 
+        if hasattr(self, "alpha") and self.transform_alpha is None:
+            self.transform_alpha = self.alpha
path: sklearn/decomposition/_dict_learning.py
text: @user1:
We don't modify the init parameters. You should set `transform_alpha = self.alpha` and then use transform_alpha. 

Since for now we don't change the behavior, you can write `transform_alpha = 1` to prepare for the future change.