Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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.

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

Conversation

bmalezieux
Copy link
Contributor

@bmalezieux bmalezieux commented Jan 12, 2021

…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?

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

sklearn/decomposition/_dict_learning.py Outdated Show resolved Hide resolved
sklearn/decomposition/_dict_learning.py Outdated Show resolved Hide resolved
sklearn/decomposition/_dict_learning.py Outdated Show resolved Hide resolved
sklearn/decomposition/_dict_learning.py Outdated Show resolved Hide resolved
@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
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
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.

sklearn/decomposition/_dict_learning.py Outdated Show resolved Hide resolved
sklearn/decomposition/_dict_learning.py Outdated Show resolved Hide resolved
sklearn/decomposition/_dict_learning.py Outdated Show resolved Hide resolved
Base automatically changed from master to main January 22, 2021 10:53
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.

sklearn/decomposition/_dict_learning.py Outdated Show resolved Hide resolved
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

sklearn/decomposition/_dict_learning.py Outdated Show resolved Hide resolved
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.

sklearn/decomposition/_dict_learning.py Outdated Show resolved Hide resolved
doc/whats_new/v1.0.rst Outdated Show resolved Hide resolved
@rth rth merged commit 66560d6 into scikit-learn:main Feb 1, 2021
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
12 tasks
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.

Dictionary Learning: transform_alpha default is not equal to alpha ?
3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.