Draft implementation of MiniBatchNMF #13326
Conversation
|
In the paper the mini batch algorithms is actually not that much faster surprisingly. That's kind of odd. |
|
The paper is the following: Online algorithms for nonnegative matrix factorization with the Itakura-Saito divergence, A Lefevre, F Bach, C Févotte |
| self.W, self.A, self.B = self._init_W(X) | ||
| # self.rho = self.r**(self.batch_size / n_samples) | ||
| # else: | ||
| # not implemented yet |
There was a problem hiding this comment.
We'll have to implement for dense.
| return H | ||
|
|
||
|
|
||
| def mini_batch(iterable1, iterable2, n=1): |
There was a problem hiding this comment.
Can you use sklearn.utils.gen_batches instead of this function?
| Ht_W = _special_sparse_dot(Ht, W, Vt) | ||
| Ht_W_data = Ht_W.data | ||
| np.divide(Vt.data, Ht_W_data, out=Ht_W_data, where=(Ht_W_data != 0)) | ||
| self.rho = self.r ** (1 / (iter + 1)) |
There was a problem hiding this comment.
Any parameter that is not specified by the init, but rather a consequence of the fit should have a name that ends with _: self.rho -> self.rho_. It should also be the case of many other variables in this object.
| # not implemented yet | ||
|
|
||
| n_batch = (n_samples - 1) // self.batch_size + 1 | ||
| self.iter = 1 |
There was a problem hiding this comment.
This variable should probably be named "self.n_iter_"
| if i == n_batch-1: | ||
| W_change = np.linalg.norm( | ||
| self.W - W_last) / np.linalg.norm(W_last) | ||
| if (W_change < self.tol) and (iter >= self.min_iter - 1): |
There was a problem hiding this comment.
"iter" should be renamed to "n_iter" as "iter" is a function in Python.
|
Can you also start working on tests. |
|
@pcerda : can you add your benchmarking code. We'll need it to profile / benchmark. |
…ent) and benchmart file (WIP)
| ngram_range: tuple, default=(2, 4) | ||
|
|
||
| init: str, default 'k-means++' | ||
| Initialization method of the W matrix. |
There was a problem hiding this comment.
perhaps add 2-3 words on what the W matrix is in the NMF formulation
|
|
||
| Parameters | ||
| ---------- | ||
|
|
There was a problem hiding this comment.
Isn't an empty line here against convention ?
|
|
||
| def __init__(self, n_components=10, batch_size=512, | ||
| r=.001, init='k-means++', | ||
| tol=1E-4, min_iter=2, max_iter=5, ngram_range=(2, 4), |
There was a problem hiding this comment.
tol does not match the default mentioned in docstring
| Ht_out = Ht * safe_sparse_dot(Ht_W, W_WT1.T) | ||
| squared_norm = np.linalg.norm( | ||
| Ht_out - Ht) / (np.linalg.norm(Ht) + 1E-10) | ||
| Ht[:] = Ht_out |
There was a problem hiding this comment.
Is the [:] to make sure that the same Ht object is used in each iteration ?
| self.W_, self.A_, self.B_ = self._m_step( | ||
| X[slice], self.W_, self.A_, self.B_, H[slice], self.iter) | ||
| self.iter += 1 | ||
| if i == n_batch-1: |
There was a problem hiding this comment.
PEP8 ? My feel is to add spaces around "-"
|
|
||
| Parameters | ||
| ---------- | ||
| X : array-like (str), shape [n_samples,] |
There was a problem hiding this comment.
n_feature missing in [] bracket ?
| break | ||
| previous_error = error | ||
| if beta_loss < 1: | ||
| W[slice][W[slice] < np.finfo(np.float64).eps] = 0. |
There was a problem hiding this comment.
consider splitting into two lines for ++readability
| @@ -1297,6 +1328,37 @@ def fit(self, X, y=None, **params): | |||
| self.fit_transform(X, **params) | |||
| return self | |||
|
|
|||
| def partial_fit(self, X, y=None, **params): | |||
|
What's the relationship between this PR and #13386? |
|
I think that #13386 is a second design that we thought was cleaner. @pcerda, can you confirm. We'll need to finish this, by the way :). Maybe @TwsThomas will be able to pitch in. |
|
Closing in favor of #16948. |
Reference Issues/PRs
What does this implement/fix? Explain your changes.
Implementation of a mini-batch NMF with multiplicative updates.
#13308
Any other comments?