-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
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
Conversation
There was a problem hiding this 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
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>
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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.
…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?