New stop condition: use component relative change #83
Conversation
Previously, convergence was achieved when `|W[i, j]-preW[i, j]|<=tol for all i, j`. Running NMF on `10*X` (where `X` is the data matrix ) would increase the number of iterations compared to NMF on `X`. Now, `||W[:, j]-preW[:, j]||/||W[:, j]+preW[:, j]||<=tol for all j` is the new convergence criterion, which is relative difference rather absolute difference and thus, it is scale invariant.
There was a problem hiding this comment.
I'll plan to merge this by the end of this week, unless objections arise. So everyone knows, @youdongguo and I collaborate and he and I discussed this in advance; I think this is an important change (I think the old criterion makes no sense at all), but anyone who cares should come to their own conclusion.
Personally, the only part I'm unsure about is whether this qualifies as breaking. Is changing a convergence criterion (which could change the number of iterations and thus both the time needed and quality of a solution) "breaking"? I'm not alone in wrestling with this, e.g., https://dev.to/turnerj/should-behavioural-changes-be-considered-breaking-changes-under-semver-2d5n#comment-b6jd.
If we do decide it's breaking, it's a little funny to release 2.0 so shortly after 1.0. But if that's what we need to do, that's fine, as version numbers are cheap.
The `verbose=true` option was broken in #83, but it wasn't detected because it was never tested.
The `verbose=true` option was broken in #83, but it wasn't detected because it was never tested.
Previously, convergence was achieved when
|W[i, j]-preW[i, j]|<=tol for all i, j.Running NMF on
10*X(whereXis the data matrix ) would increase the number of iterations compared to NMF onX. Now,||W[:, j]-preW[:, j]||/||W[:, j]+preW[:, j]||<=tol for all jis the new convergence criterion, which is relative difference rather absolute difference and thus, it is scale invariant.This might be a breaking change but it is unclear.
This new criterion is more similar to the the stop condition in
sklearn.decomposition.NMF