10000 DOC Added meaning of default=None for n_components in MiniBatchSparsePCA and MiniBatchDictionaryLearning by alceballosa · Pull Request #21428 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

DOC Added meaning of default=None for n_components in MiniBatchSparsePCA and MiniBatchDictionaryLearning #21428

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 25, 2021
Merged

DOC Added meaning of default=None for n_components in MiniBatchSparsePCA and MiniBatchDictionaryLearning #21428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
41e0e84
Added description for the default=None case of the n_components param…
alceballosa Oct 23, 2021
fbebd5f
Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…
alceballosa Oct 23, 2021
3eddba1
Add the description for the n_components=None case to MiniBatchDictio…
alceballosa Oct 23, 2021
5c12d98
Setting the reference to the number of features back to ``n_features`…
alceballosa Oct 23, 2021
a4a72f6
Update _sparse_pca.py
alceballosa Oct 23, 2021
ffa9e91
Create _dict_learning.py
alceballosa Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions sklearn/decomposition/_dict_learning.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,8 +762,9 @@ def dict_learning_online(
X : ndarray of shape (n_samples, n_features)
Data matrix.

n_components : int, default=2
Number of dictionary atoms to extract.
n_components : int or None, default=2
Number of dictionary atoms to extract. If None, then ``n_components``
Copy link
Member

Choose a reason for hiding this comment

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

Then we should also correct the line above

n_components : int or None, default=2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thank you! I just did the relevant edit. Let me know if anything else looks off.

is set to ``n_features``.

alpha : float, default=1
Sparsity controlling parameter.
Expand Down Expand Up @@ -1335,8 +1336,9 @@ class DictionaryLearning(_BaseSparseCoding, BaseEstimator):

Parameters
----------
n_components : int, default=n_features
Number of dictionary elements to extract.
n_components : int, default=None
Number of dictionary elements to extract. If None, then ``n_components``
Copy link
Member

Choose a reason for hiding this comment

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

Actually, the above line is wrong. It should be

n_components : int, default=None

is set to ``n_features``.

alpha : float, default=1.0
Sparsity controlling parameter.
Expand Down
5 changes: 3 additions & 2 deletions sklearn/decomposition/_sparse_pca.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class SparsePCA(TransformerMixin, BaseEstimator):
----------
n_components : int, default=None
Number of sparse atoms to extract. If None, then ``n_components``
is set to ``n_features_in_``.
is set to ``n_features``.

alpha : float, default=1
Sparsity controlling parameter. Higher values lead to sparser
Expand Down Expand Up @@ -249,7 +249,8 @@ class MiniBatchSparsePCA(SparsePCA):
Parameters
----------
n_components : int, default=None
Number of sparse atoms to extract.
Number of sparse atoms to extract. If None, then ``n_components``
is set to ``n_features``.

alpha : int, default=1
Sparsity controlling parameter. Higher values lead to sparser
Expand Down
0