Skip to content

NMF default init with 'mu' solver #18505

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

Closed
jeremiedbb opened this issue Oct 1, 2020 · 3 comments · Fixed by #18525
Closed

NMF default init with 'mu' solver #18505

jeremiedbb opened this issue Oct 1, 2020 · 3 comments · Fixed by #18525
Assignees
Labels

Comments

@jeremiedbb
Copy link
Member

jeremiedbb commented Oct 1, 2020

This issue discovered in #16948 has 2 parts.

  • The default init for NMF is None, which corresponds to "nndsvd". However there's this warning
if solver == 'mu' and init == 'nndsvd':
        warnings.warn("The multiplicative update ('mu') solver cannot update "
                      "zeros present in the initialization, and so leads to "
                      "poorer results when used jointly with init='nndsvd'. "
                      "You may try init='nndsvda' or init='nndsvdar' instead.",
                      UserWarning)

which is not issued when init is left to None. If it's discouraged to use nndsvd with the multiplicative update, don't you think we should change the default to be nndsvda when solver == 'mu' ? We could keep None or name it "auto".

  • There's a discrepancy between fit.transform and fit_transform, especially with 'mu' solver.
    The current default for the init being nndsvd, W likely has zeros in its initialization which are propagated all the way using fit_transform. Using fit.transform is different because for transform, W is initialized as a constant matrix.
    Maybe changing the default init to nndsvda can be considered enough. Otherwise I wonder if transform should initialize W using the same function as fit. wdyt ?
    edit: changing the default init still fails the common test fit.transform vs fit_transform for the multiplicative update solver. Using the same init at transform also does not work.

ping @cmarmo @TomDLT

@TomDLT
Copy link
Member

TomDLT commented Oct 1, 2020

  • Initialization with "nndsvd" is bad for solver="mu", we should have "nndsvdar". Actually it was intended to use "nndsvdar" in [MRG+1] refactor NMF and add CD solver #4852, as seen in the original docstring here, but for some reason the implementation used "nndsvd" (my bad). We should definitely change the default.
  • The discrepancy between fit.transform and fit_transform is hard to avoid in NMF. To completely fix it, we could call transform at the end of fit_transform, but this is quite costly.

@cmarmo cmarmo added the Bug label Oct 1, 2020
@cmarmo
Copy link
Contributor

cmarmo commented Oct 1, 2020

Thanks @TomDLT! Do you mind if I take care of this one? This will help me in understanding the 'big picture'.

@cmarmo cmarmo removed the Bug: triage label Oct 1, 2020
@cmarmo
Copy link
Contributor

cmarmo commented Oct 1, 2020

take

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants