8000 DOC Specify the meaning of dict_init=None in sklearn.decomposition.dict_learning_online by marenwestermann · Pull Request #23261 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

DOC Specify the meaning of dict_init=None in sklearn.decomposition.dict_learning_online #23261

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

Merged
merged 3 commits into from
May 4, 2022

Conversation

marenwestermann
Copy link
Member

Reference Issues/PRs

Towards #17295

What does this implement/fix? Explain your changes.

I added documentation to the dict_learning_online function in sklearn/decomposition/_dict_learning.py for the case dict_init=None.

Any other comments?

In #17295 it is said that dict_learning_online also has the attribute code_init which also needs further documentation. However, this is a mistake as dict_learning_online doesn't have this attribute.

Comment on lines 813 to 815
using :func:`randomized_svd` as::

_, S, dictionary = randomized_svc(X, n_components, random_state)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
using :func:`randomized_svd` as::
_, S, dictionary = randomized_svc(X, n_components, random_state)
using :func:`~sklearn.utils.randomized_svd` as::
_, S, dictionary = randomized_svd(X, n_components, random_state)

Copy link
Member
@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @marenwestermann. I think it's good to say that the initial dict is built from an svd of the data, but I don't think it's necessary to expose the corresponding lines of code.

@glemaitre
Copy link
Member

Thanks for the PR @marenwestermann. I think it's good to say that the initial dict is built from an svd of the data, but I don't think it's necessary to expose the corresponding lines of code.

We actually discussed it yesterday during office hours and it was my feedback as well.

@marenwestermann
Copy link
Member Author

Thank you for your feedback. I updated the PR.

Initial value for the dictionary for warm restart scenarios.
Initial values for the dictionary for warm restart scenarios.
If `None`, the initial values for the dictionary are created
using :func:`~sklearn.utils.randomized_svd`.
Copy link
Member
@glemaitre glemaitre May 4, 2022

Choose a reason for hiding this comment

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

Suggested change
using :func:`~sklearn.utils.randomized_svd`.
with an SVD decomposition of the data via :func:`~sklearn.utils.randomized_svd`.

Copy link
Member
@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member
@jeremiedbb jeremiedbb left a comment

LGTM. Thanks @marenwestermann

@jeremiedbb jeremiedbb merged commit 125a6c6 into scikit-learn:main May 4, 2022
@marenwestermann marenwestermann deleted the dict-learning branch May 5, 2022 07:39
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0