ENH Warn future change of default init in NMF

[cmarmo]

Reference Issues/PRs

Closes #18505

What does this implement/fix? Explain your changes.

• Make 'nndsvda' default init in tests.
• Prepare default change in 0.26

Any other comments?

To make tests pass I had to increase the tolerance for float32 float64 consistency.

Ping @jeremiedbb, @TomDLT

[TomDLT]

It should be noted that the current PR will break backward compatibility:

• raising an error when using solver="mu" and init="nndsvd" explicitly
• changing the results with both solvers when using init=None. The changes will probably be small with the "cd" solver, but might be larger with the "mu" solver (though this might be considered a bugfix).
  I am fine with it, but other maintainers might disagree.

Other suggested changes:

• add Not available for solver="mu". in the nndsvd section of the docstrings (in _initialize_nmf, NMF, and non_negative_matrix_factorization).
• add a whats_new entry, including in the "Changed models" section.

---

Another possibility is to keep allowing init="nndsvd" with solver="mu" (the fixed sparsity might be considered as a kind of regularization), keep the warning, and change the behavior of init=None through a deprecation process.

Not sure what is the best solution, to be honest.

[GaelVaroquaux]

If we go ahead with this change, a clear note in the whats_new should be added

[GaelVaroquaux]

The proposed backward incompatible change may be against our guidelines. The problem being that people need to be able to update scikit-learn without worrying about breakage.

An argument for going ahead with such backward-incompatible change would be if the prior version should be considered as a bug. In other terms, if it leads often to significant breakage, it can be changed without a compatibility shim.

Trying to summarize things to make sure that I understand: in the current settings, the "transform" is broken by default, and the change is required to fix things? Am I correct?

[jeremiedbb]

Trying to summarize things to make sure that I understand: in the current settings, the "transform" is broken by default, and the change is required to fix things? Am I correct?

This will not completely fix the fit.transform vs fit_transform issue. It's just a better default for the multiplicative update solver because it avoids zeros in the init.

[jeremiedbb]

This PR currently changes the default for both solvers. Shouldn't it be only for the 'mu' solver ?

I'm in favor of keeping the warning and change the default for nmf through a deprecation cycle. (Note that for MiniBatchNMF you can already set the good default).

[ogrisel]

I think I like the last suggestion of @TomDLT's #18525 (review) most:

Another possibility is to keep allowing init="nndsvd" with solver="mu" (the fixed sparsity might be considered as a kind of regularization), keep the warning, and change the behavior of init=None through a deprecation process.

[ogrisel]

I believe this is the same solution as @jeremiedbb's #18525 (comment).

[cmarmo]

Thanks for all your comments.
I have reverted my changes and deprecated init='nndsvd'.

[jeremiedbb]

Maybe we could take the opportunity to rename it "auto" (in 2 versions).

You also need to change the default in _initialize_nmf and maybe elsewhere
Could also make the warning visible (i.e even when init is none) in _check_string_parameters ?

[TomDLT]

I think we should change from 'nndsvd' to 'nndsvda' for all solvers. Indeed, I think it makes almost no difference for the "cd" solver, and it simplifies the behavior/documentation.

[TomDLT]

The (non negative) SVD initialization:

• is used directly when init="nndsvd"
• changes zeros into X.mean() when init="nndsvda"
• changes zeros into random values when init="nndsvdar"

Importantly, the "mu" solver is based on multiplicative updates, so zeros in the initialization remain zeros during optimization. This is why init="nndsvd" should not be the default, and the reason of this PR.

[cmarmo]

I need some direction here.
The test test_nmf_regularization with solver='mu' and l1_ratio=1., fails when NMF is initialized with 'nndsvda' while passes when initialized with 'nndsvd'. I have to increase alpha to 2. in order to make the test pass again for all the combinations of solver and l1_ratio. The failing assertion is:

scikit-learn/sklearn/decomposition/tests/test_nmf.py

Line 484 in 39427c3

assert W_model.mean() > W_regul.mean()

I guess this is due to the fact that (quoting @TomDLT) "initialization changes zeros into X.mean() when init="nndsvda"?
Should I keep 'nndsvd' here as we want to separate the effect of the initialization from the one of regularization?

[jeremiedbb]

It's failing for both 'cd' and 'mu'. I wonder if the test is correct. I don't think it's guaranteed that the mean of W and the mean of H should both be smaller when using l2 reg. Don't you think we should check that ||W||^2 + ||H||^2 is smaller when using l2 reg instead ?

[cmarmo]

I wonder if the test is correct

I'm not the right person to ask... :)
Anyway, increasing max_iter fixes the other failing tests, so test_nmf_regularization is the last one to check.

[TomDLT]

I don't think it's guaranteed that the mean of W and the mean of H should both be smaller when using l2 reg. Don't you think we should check that ||W||^2 + ||H||^2 is smaller when using l2 reg instead ?

You are right, the test is wrong, we should check what you suggest.

• The test is looking at W.mean() (L1 norm) whereas it should consider squared_norm(W) (L2 norm).
• The test is looking at W and H independently, while it should consider the sum of their norms. To support this, you can see in the first figure in [MRG] Add scaling to alpha regularization parameter in NMF #5296 that increasing the regularization can sometime increase the mean of either W or H, but not both.

[cmarmo]

Thanks @jeremiedbb and @TomDLT, I will change the test then: is this worth a new PR?

[jeremiedbb]

it's fine to do it in this pr

[ogrisel]

I seems that all the comments have been tackled. LGTM. Thanks for the fix @cmarmo and thanks @TomDLT for the interesting comments.

[TomDLT]

Thanks @cmarmo !

[vene]

Thanks for this and for catching the incorrect test! Just a note, there is a leftover comment in the test file that is now outdated (https://github.com/scikit-learn/scikit-learn/pull/18525/files#diff-e809907f1fe6840a81b7b9b9f1d0ef494052df97d328eeb25ae3e0495e238575R470) and it might confuse devs in the future.

(I probably introduced this mistake...)

[cmarmo]

Just a note, there is a leftover comment in the test file that is now outdated (https://github.com/scikit-learn/scikit-learn/pull/18525/files#diff-e809907f1fe6840a81b7b9b9f1d0ef494052df97d328eeb25ae3e0495e238575R470) and it might confuse devs in the future.

Right! Thanks @vene! I will fix it in #16948 if there are no objections.