[MRG] Fixed NMF IndexError

[zjpoh]

Reference Issues/PRs

Fixes #11650

What does this implement/fix? Explain your changes.

1. I changed the condition for using nndsvd to initialize NMF to k <= min(m,n) and raised an ValueError if k > min(m,n) and init = 'nndsvd'.
   Why?
   According to the table 1 of SVD based initialization paper, for matrix $A\in\mathbb{R}^{m\times n}_+$, the condition for nndsvd initialization is k < min(m, n). However, in the code, nndsvd is used when k < n instead of k < min(m, n).
   Note that in the code, k = n_components, m = n_samples and n = n_features.
   In addition, from my understanding of the paper, k <= min(m,n) is a sufficient condition because the initialization method is based on SVD and SVD only requires k <= min(m, n) instead of k < min(m, n). I have verified that setting k <= min(m, n) gives the correct solution and passes all unit tests.

2. I set init = None as the default parameter of non_negative_factorization.
   Why?
   non_negative_factorization has init = 'random' [here] while _initialize_nmf sets init = 'nndsvd' only if it is called withinit = None [here]. That is, the calculation of non_negative_factorization with the default parameter will never use init = 'nndsvd'. Hence, the output of non_negative_factorization and NMF class will be different unless init = 'random' is passed as a parameter to NMF.
   Note: non_negative_factorization have init = 'random' as the default parameter is also inconsistent with the documentation.

Any other comments?

I am aware that the changes that I made are inconsistent with the suggestions in the issue. But this is what I found out after reading the code and the paper. Please let me know if this makes sense. Thanks.

[jnothman]

Please add a test

[TomDLT]

LGTM except nitpicks.

Can you also add a bugfix entry in doc/whats_new/v0.20.rst?

Thanks !

[TomDLT]

The NNDSVD is performed also with init = 'nndsvda' and init = 'nndsvdar', which also need the same constraint.
You can use if init != 'random' to handle all three cases.
Please also update your unit test accordingly.

[zjpoh]

Good point. Thanks!

[jnothman]

Why are we changing this default init?

[zjpoh]

Sorry for the super late reply and thanks for pointing this out. I should have explain it better in the PR notes.

I made this changes because the documentation says here

        Default: 'nndsvd' if n_components <= min(n_samples, n_features),
            otherwise random.

But in init is set to 'nndsvd' or 'random' only if init is None. See here

    if init is None:
        if n_components <= min(n_samples, n_features):
            init = 'nndsvd'
        else:
            init = 'random'

[jnothman]

Then we need to update the documentation to reflect the true default, not make a backwards-incompatible change. You are welcome to update the documentation in a separate, focused PR.

[zjpoh]

Got it. Let me revert that and create a new PR on that.

[jnothman]

always use init is not None rather than just testing init as a bool

[jnothman]

This is in the wrong place and should be prefixed by |Fix|

[zjpoh]

@jnothman Thank you for your quick turnaround.

The test is failing because NMF and non_negative_factorization have different default init. NMF has init=None while non_negative_factorization has init='random'.

Per your comment above, we want to update the documentation instead of making a backwards-incompatible change. However, not changing the code means that NMF and non_negative_factorization are inconsistent.

I am wondering what is your suggestion on this.

[jnothman]

I am okay to leave them inconsistent for now, or we could change the default in `non_negative_factorization` with a deprecation process (see http://scikit-learn.org/dev/developers/contributing.html#change-the-default-value-of-a-parameter ).

[jnothman]

Tests are now failing

[jnothman]

use :user:

[jnothman]

Thanks @zjpoh !