8000 REFACTOR: dict_learning and dict_learning_online should be merged into a single func · Issue #9009 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

REFACTOR: dict_learning and dict_learning_online should be merged into a single func #9009

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

Jump to bottom
Closed
dohmatob opened this issue Jun 6, 2017 · 1 comment
Closed

REFACTOR: dict_learning and dict_learning_online should be merged into a single func #9009

dohmatob opened this issue Jun 6, 2017 · 1 comment
Labels
module:decomposition

Comments

@dohmatob
Copy link
Contributor
dohmatob commented Jun 6, 2017 •
edited
Loading

Description

  • There is code / logic duplication between decomposition.dict_learning.py.dict_learning and dict_learning_online. Formally, the code should be equivalent except, that

    • for the former the codes should be computed all at once (batch_size = n_samples) and this process iterated (alternating BCD, as it's already done), whilst
    • for the latter the each sample point's code is computed as the sample is pooled-in. The dictionary update function should the same in both case (Alg2 of the ref paper.)

  • As a side effect of this code duplication, there prototype of backend _update_dict function is somewhat "dangling". E.g the shape of argument code can be (n_components, n_features) or (n_components, n_features) depending on whether it's called from within dict_learning or dict_learning_online resp.

Proposal

Have a single function dict_learning_online(batch_size=some_default) which reproduces full-batch mode (the current dict_learning function) when batch_size == n_samples.
@dohmatob dohmatob mentioned this issue Jun 7, 2017
Closed
@dohmatob dohmatob changed the title DUPLICATE: dict_learning and dict_learning_online should be merged into a single func REFACTOR: dict_learning and dict_learning_online should be merged into a single func Jun 7, 2017
@jeremiedbb
Copy link
Member

As explained in #9036, MiniBatchDictionaryLearning has been refactored and no longer calls dict_learning_online (it's now the opposite). The inconsistencies in the _dict_update function have also been fixed. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
module:decomposition
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dohmatob @cmarmo @jeremiedbb
0