8000 FIX Make dict learning loss match the paper by jeremiedbb · Pull Request #19210 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

FIX Make dict learning loss match the paper #19210

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
Merged
merged 15 commits into from
Oct 27, 2021
Merged

FIX Make dict learning loss match the paper #19210

merged 15 commits into from
Oct 27, 2021

Conversation

jeremiedbb
Copy link
Member

Built on top of #19198 for convenience.

In the paper

J. Mairal, F. Bach, J. Ponce, G. Sapiro, 2009: Online dictionary learning  
for sparse coding (https://www.di.ens.fr/sierra/pdfs/icml09.pdf)

the constraint is that the norm of each atom is less or equal to 1. The current doc and implementation make it equal to 1. I'm not sure what it implies on the optimization problem but it might be considered a bug.

SparsePCA relies on dict learning and has the same issue. In the paper

R. Jenatton, G. Obozinski, F. Bach, 2009: Structured Sparse Principal Component
Analysis (https://www.di.ens.fr/~fbach/sspca_AISTATS2010.pdf)

the constraint on the components is also less or equal to 1.

This change was also proposed in #9036

Base automatically changed from master to main January 22, 2021 10:53
@ogrisel
Copy link
Member
ogrisel commented Apr 15, 2021

@jeremiedbb this PR can now be rebased on top of the main branch now that #19198 has been merged.

@ogrisel
Copy link
Member
ogrisel commented Jun 3, 2021

How does this impact qualitatively the existing examples?

Apparently no tests were impacted by this change. I am not sure this is a good sign ;)

ogrisel
ogrisel approved these changes Jun 3, 2021
View reviewed changes
Copy link
Member
@ogrisel ogrisel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Assuming it does not impact the example, fine with me but this needs to be documented in the changelog.

@jeremiedbb
Copy link
Member Author

Here's the result of the plot_faces_decomposition.py example.
before:
dl_before
after:
dl_after

there are some variations but nothing to worry I guess.

@glemaitre glemaitre changed the title Make dict learning loss match the paper FIX Make dict learning loss match the paper Oct 27, 2021
@glemaitre glemaitre added this to the 1.0.2 milestone Oct 27, 2021
@glemaitre glemaitre merged commit e773581 into scikit-learn:main Oct 27, 2021
@glemaitre
Copy link
Member

Thanks @jeremiedbb

samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@jeremiedbb @ogrisel @samronsin
FIX Make dict learning loss match the paper (scikit-learn#19210)
2d25c58 
Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Dec 24, 2021
@jeremiedbb @ogrisel @glemaitre
FIX Make dict learning loss match the paper (scikit-learn#19210)
3dd5758 
Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>
glemaitre pushed a commit that referenced this pull request Dec 25, 2021
@jeremiedbb @ogrisel @glemaitre
FIX Make dict learning loss match the paper (#19210)
e437a9c 
Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

Assignees
No one assigned
Labels
module:decomposition
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jeremiedbb @ogrisel @glemaitre
0