Skip to content

[MRG] NMF and non_negative_factorization have inconsistent default init #12989

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 11 commits into from
Jan 30, 2019
Merged

[MRG] NMF and non_negative_factorization have inconsistent default init #12989

merged 11 commits into from
Jan 30, 2019

Conversation

zjpoh
Copy link
Contributor

@zjpoh zjpoh commented Jan 16, 2019

Reference Issues/PRs

Fixes #12988. See also #11667.

What does this implement/fix? Explain your changes.

Changed the default in non_negative_factorization with a deprecation process.

jnothman
jnothman reviewed Jan 16, 2019
View reviewed changes
sklearn/decomposition/nmf.py Outdated
@@ -986,6 +986,11 @@ def non_negative_factorization(X, W=None, H=None, n_components=None,
factorization with the beta-divergence. Neural Computation, 23(9).
"""

if init == "warn":
warnings.warn("The default value of init will change from "
"random to None in 0.22.", FutureWarning)
Copy link
Member

Choose a reason for hiding this comment

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

0.23

Copy link
Member

Choose a reason for hiding this comment

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

The behaviour won't change if n_components >= n_features so we could add that condition

jnothman
jnothman approved these changes Jan 19, 2019
View reviewed changes
Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Thanks!

Please add an |API| entry to the change log at doc/whats_new/v0.21.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

@jnothman
Copy link
Member

Sorry I shouldn't have approved: tests are failing. Maybe I've not reviewed correctly

jnothman
jnothman requested changes Jan 19, 2019
View reviewed changes
Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Tests failing

@jnothman
Copy link
Member

Travis still unhappy. Please remember to rename to MRG when you think this is safe to merge

@zjpoh
Copy link
Contributor Author

zjpoh commented Jan 22, 2019

Sure. I'm still figuring out how to make Travis happy. Will change the title after figuring it out.

@zjpoh
Copy link
Contributor Author

zjpoh commented Jan 23, 2019

@jnothman Travis is happy now.

@zjpoh zjpoh changed the title [WIP] NMF and non_negative_factorization have inconsistent default init [MRG] NMF and non_negative_factorization have inconsistent default init Jan 23, 2019
jnothman
jnothman reviewed Jan 23, 2019
View reviewed changes
sklearn/decomposition/tests/test_nmf.py
@@ -380,8 +382,8 @@ def test_nmf_negative_beta_loss():

def _assert_nmf_no_nan(X, beta_loss):
W, H, _ = non_negative_factorization(
X, n_components=n_components, solver='mu', beta_loss=beta_loss,
random_state=0, max_iter=1000)
X, init='random', n_components=n_components, solver='mu',
Copy link
Member

Choose a reason for hiding this comment

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

we should probably make sure that init is explicitly set elsewhere in the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Added explicit init to all tests other than the consistency check test between NMF and non_negative_factorization.

qinhanmin2014
qinhanmin2014 reviewed Jan 26, 2019
View reviewed changes
Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

Choose a reason for hiding this comment

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

Please add the reason of the deprecation in what's new and the deprecation message (e.g., to keep consistent with decomposition.NMF).
please explain init=None in the docstring of non_negative_factorization.
please add init=None to possible options and explain it in the docstring of NMF.

doc/whats_new/v0.21.rst Outdated
:func:`decomposition.non_negative_factorization` will change from
:code:`random` to :code:`None` to match :class:`decomposition.NMF`
in version 0.23. A FutureWarning is raised when the default value
is used. :issue:`12988` by :user:`Zijie (ZJ) Poh <zjpoh>`.
Copy link
Member

Choose a reason for hiding this comment

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

redundant space

qinhanmin2014
qinhanmin2014 approved these changes Jan 29, 2019
View reviewed changes
sklearn/decomposition/nmf.py Outdated
if init == "warn":
if n_components < n_features:
warnings.warn("The default value of init will change from "
"random to None in 0.23.", FutureWarning)
Copy link
Member

Choose a reason for hiding this comment

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

please also mention the reason in the deprecation message

qinhanmin2014
qinhanmin2014 approved these changes Jan 30, 2019
View reviewed changes
Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

Choose a reason for hiding this comment

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

thanks @zjpoh

@qinhanmin2014 qinhanmin2014 merged commit bb3f93c into scikit-learn:master Jan 30, 2019
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Jan 30, 2019
@zjpoh @glemaitre
API NMF and non_negative_factorization have inconsistent default init (…
9445eb1 
…scikit-learn#12989)
@zjpoh zjpoh deleted the change_nmf_init branch February 1, 2019 17:06
thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 6, 2019
@zjpoh @thomasjpfan
API NMF and non_negative_factorization have inconsistent default init (…
c85b704 
…scikit-learn#12989)
thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 7, 2019
@zjpoh @thomasjpfan
API NMF and non_negative_factorization have inconsistent default init (…
999d8fa 
…scikit-learn#12989)
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
@zjpoh
API NMF and non_negative_factorization have inconsistent default init (…
6966d73 
…scikit-learn#12989)
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "API NMF and non_negative_factorization have inconsistent defa…
c0df5fd 
…ult init (scikit-learn#12989)"

This reverts commit 6966d73.
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "API NMF and non_negative_factorization have inconsistent defa…
b926c30 
…ult init (scikit-learn#12989)"

This reverts commit 6966d73.
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@zjpoh @koenvandevelde
API NMF and non_negative_factorization have inconsistent default init (…
23f0b58 
…scikit-learn#12989)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman approved these changes

@qinhanmin2014 qinhanmin2014 qinhanmin2014 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NMF and non_negative_factorization have inconsistent default init
3 participants
@zjpoh @jnothman @qinhanmin2014