Skip to content

Dictionary Learning: transform_alpha default equal to alpha #19159

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 13 commits into from
Feb 1, 2021
Merged

Dictionary Learning: transform_alpha default equal to alpha #19159

merged 13 commits into from
Feb 1, 2021

Conversation

bmalezieux
Copy link
Contributor

@bmalezieux bmalezieux commented Jan 12, 2021 •
edited
Loading

…ctionaryLearning

Reference Issues/PRs

Fixes #19154

What does this implement/fix? Explain your changes.

Setting the default value of transform_alpha to alpha in order to avoid confusion.

Any other comments?

jeremiedbb
jeremiedbb reviewed Jan 18, 2021
View reviewed changes
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 @bmalezieux. Please update the title of the PR to better reflect the changes

@bmalezieux bmalezieux changed the title Precising that transform_alpha is not equal to alpha by default in Di… Dictionary Learning: transform_alpha default equal to alpha Jan 18, 2021
bmalezieux and others added 3 commits January 21, 2021 12:57
@bmalezieux
Changing transform_alpha in _BaseSparseCoding._transform
7c4e200
@bmalezieux @jeremiedbb
Update sklearn/decomposition/_dict_learning.py
85da8e2 
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
@bmalezieux @jeremiedbb
Update sklearn/decomposition/_dict_learning.py
d9767cc 
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
jeremiedbb
jeremiedbb reviewed Jan 21, 2021
View reviewed changes
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.

This kind of change requires a 2 release future warning. For now we should just issue a future warning when transform_alpha is None and alpha != 1 saying that the default will change in version 1.2.

Also Please add an entry in the v1.0 what's new.

Base automatically changed from master to main January 22, 2021 10:53
jeremiedbb
jeremiedbb reviewed Jan 25, 2021
View reviewed changes
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.

There are tests failing because we change future warnings into errors in our test suite. We need to set transform_alpha in the failing tests.

Note that first let's only issue the warning when alpha !=1 (see comment below). This should significantly reduce the number of failing tests.

jeremiedbb
jeremiedbb approved these changes Jan 26, 2021
View reviewed changes
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.

LGTM. Just a nitpick. Thanks @bmalezieux

rth
rth approved these changes Feb 1, 2021
View reviewed changes
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

A few minor comments otherwise LGTM. This indeed makes more sense than a default to None that maps to 1.0.

@rth rth merged commit 66560d6 into scikit-learn:main Feb 1, 2021
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rth rth rth approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

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

Successfully merging this pull request may close these issues.

Dictionary Learning: transform_alpha default is not equal to alpha ?
3 participants
@bmalezieux @rth @jeremiedbb