DOC Added meaning of default=None for n_components in MiniBatchSparsePCA and MiniBatchDictionaryLearning #21428
Conversation
…eter in MiniBatchSparsePCA.
…nto minibatch_sparse_pca-n_components_None
…naryLearning class and dict_learning_online function.
| @@ -1336,7 +1337,8 @@ class DictionaryLearning(_BaseSparseCoding, BaseEstimator): | |||
| Parameters | |||
| ---------- | |||
| n_components : int, default=n_features | |||
| Number of dictionary elements to extract. | |||
| Number of dictionary elements to extract. If None, then ``n_components`` | |||
| is set to ``n_features_in_``. | |||
There was a problem hiding this comment.
I think we can keep it n_features, and in particular I think it's good if the docstring is the same for all the functions and method. Sorry if there was some confusion about that.
There was a problem hiding this comment.
I agree with using n_features to be consistent with code everywhere else.
There was a problem hiding this comment.
Hi @amueller, @thomasjpfan, should I update my other PR (#21421) to use n_features instead of n_features_in_?
There was a problem hiding this comment.
@alceballosa You can open another PR to make the adjustment.
Updated references to the number of features to be ``n_features``.
|
Hi @amueller and @thomasjpfan I just updated |
| @@ -1336,7 +1337,8 @@ class DictionaryLearning(_BaseSparseCoding, BaseEstimator): | |||
| Parameters | |||
| ---------- | |||
| n_components : int, default=n_features | |||
| Number of dictionary elements to extract. | |||
| Number of dictionary elements to extract. If None, then ``n_components`` | |||
There was a problem hiding this comment.
Actually, the above line is wrong. It should be
n_components : int, default=None| @@ -763,7 +763,8 @@ def dict_learning_online( | |||
| Data matrix. | |||
|
|
|||
| n_components : int, default=2 | |||
| Number of dictionary atoms to extract. | |||
| Number of dictionary atoms to extract. If None, then ``n_components`` | |||
There was a problem hiding this comment.
Then we should also correct the line above
n_components : int or None, default=2There was a problem hiding this comment.
Hi @glemaitre: quick question before editing this, since n_components for class DictionaryLearning (line 1338) can take None too, shouldn't the dtype for line 765 be something along the lines of "int or None" too? Or is that unnecessary because the default value being None already states that fact implicitly?
Thank you!
There was a problem hiding this comment.
Or is that unnecessary because the default value being None already states that fact implicitly
This is exactly the case, default=None is already making it implicit. We should only ensure to specify what it means in the docstring that follows (it is indeed what you are doing).
We would sometimes mention None in the type and as a default (e.g. random_state because the semantic of None is then different in this case).
There was a problem hiding this comment.
Great, thank you! I just did the relevant edit. Let me know if anything else looks off.
Minor changes for consistency in n_components default value and its dtype.
|
Thanks @alceballosa LGTM |
…PCA and MiniBatchDictionaryLearning (scikit-learn#21428)
…PCA and MiniBatchDictionaryLearning (scikit-learn#21428)
…PCA and MiniBatchDictionaryLearning (scikit-learn#21428)
…PCA and MiniBatchDictionaryLearning (#21428)
Reference Issues/PRs
Solves two sub-issues in #17295.
What does this implement/fix? Explain your changes.
The default case (None) for the
n_componentsparameter insklearn.decomposition.MiniBatchSparsePCAandsklearn.decomposition.MiniBatchDictionaryLearningwas not described, so I went ahead and added a short description. In addition, I noticed that thesklearn.decomposition.dict_learning_onlinefunction also has an_componentsparameter. This parameter has a default=2 value but can take None values, so I added a description for that case too.Any other comments?
#DataUmbrella sprint
cc: @amueller @thomasjpfan