refactor: migration rbm tensorflow to pytorch - #2359
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Signed-off-by: ds-wook <[email protected]>
685f404 to
6904422
Compare
miguelgfierro
left a comment
There was a problem hiding this comment.
@ds-wook really good stuff again! congrats!
I added some comments, please take a look
| "model = RBM(\n", | ||
| " possible_ratings=np.setdiff1d(np.unique(Xtr), np.array([0])),\n", | ||
| " visible_units=Xtr.shape[1],\n", | ||
| " hidden_units=600,\n", | ||
| " training_epoch=30,\n", | ||
| " minibatch_size=60,\n", | ||
| " keep_prob=0.9,\n", | ||
| " with_metrics=True\n", |
There was a problem hiding this comment.
The old implementation was not very well written. I think it would be good to improve the new one now that we are making the change.
The constructor should have only properties of the model and the trainer fit properties of the training process.
So for example, visible_units=Xtr.shape[1], hidden_units=600, is part of the model and training_epoch=30, minibatch_size=60, keep_prob=0.9, with_metrics=True, would be part of the training
| " k=K,\n", | ||
| " )\n", | ||
| "\n", | ||
| " eval_ndcg = ndcg_at_k(\n", |
There was a problem hiding this comment.
the difference in the metrics could be because of this:
Silent behavior change in Gibbs annealing (not a regression — arguably a fix). In the original TF code,
gibbs_protocol re-called self.gibbs_sampling() to rebuild self.v_k, but the optimizer op had already been built
against the initial k=1 graph, so increasing k had no effect on training. The PyTorch version uses k directly in the loop, so CD-k annealing now genuinely takes effect. This changes training dynamics vs. the TF baseline — worth a sentence in the PR body, since it (not just the dropout fix) contributes to the metric differences.
- gibbs_protocol bounds fix is correct and good. The added guard self.l + 1 < len(self.sampling_protocol) fixes a latent IndexError in the original (which indexed sampling_protocol[self.l + 1] unguarded). Good catch — but there's no test exercising it.
- num_minibatches = int(n_users / self.minibatch) drops the remainder and can be 0. If minibatch_size > n_users the training loop never executes and the model is returned untrained (rmse_train all zeros) with no warning. This matches the original, so it's not a regression, but a fail-fast check (or at least max(1, ...)) would be a cheap robustness win.
Signed-off-by: ds-wook <[email protected]>
|
@miguelgfierro I've incorporated the requested changes. Could you please review them when you have a chance? Thanks! |
| @@ -1,870 +1,906 @@ | |||
| { | |||
| "cells": [ | |||
There was a problem hiding this comment.
There is a big difference in the metrics:
There is something that is not working.
Might be that we are leaking test data into the train process?
Also, please notice that the deep dive notebook also needs to be updated: https://github.com/recommenders-team/recommenders/blob/main/examples/02_model_collaborative_filtering/rbm_deep_dive.ipynb
There was a problem hiding this comment.
Thanks for addressing the previous comments — the constructor/fit() split, the ported tests, and the new fail-fast checks all look good.
Before merging, I investigated the metric numbers in the notebook (MAP going from 0.14 to 0.53). I ran both the old TF code and this PR on the same data split. Here is what I found, in simple terms.
1. The old implementation was already wrong: it leaks the test set
The notebook evaluates with model.recommend_k_items(Xtst). This gives the model the test set as input, and then we check if it recommends those same test items. The model just has to echo its input to score well. This has been the case since the original TF notebook.
Proof: if we give the model the correct input (Xtr, the training ratings), the metrics drop to random level for both implementations:
| Input | Old TF (precision@10) | This PR (precision@10) |
|---|---|---|
Xtst (what the notebook does) |
0.32 | 0.57 |
Xtr (correct) |
0.003 | 0.014 |
So neither 0.14 nor 0.53 is a real number. The PyTorch model scores higher only because it echoes its input better, not because it recommends better. On top of that, map_at_k changed definition in 2023 (#2004), which inflates the new MAP even more. The dropout explanation in the PR description is not the cause — I tested it, and forcing dropout at inference barely changes the results. Please remove the improvement claim from the PR description and the notebook.
2. The implementation does not match the paper
I also evaluated the model the way the paper does (predict the held-out ratings, measure RMSE). The result: the model is worse than just predicting the average rating for everything.
| Predictor | Test RMSE (ML100k) |
|---|---|
| Always predict the global mean | 1.13 |
| Item mean | 1.03 |
| RBM (this PR, notebook config) | 1.67 |
| Paper's RBM (Netflix) | 0.91, beating the 0.95 baseline |
The reason is that our implementation simplifies the paper too much:
- The paper uses one-hot softmax visible units: 5 binary units per item, with separate weights and biases per rating value. Our code uses a single scalar unit per item with
p(v=l) ∝ exp(l·φ). This forces the rating distribution to be monotone — the model cannot even represent "most people rate this movie a 3". multinomial_samplingis not real categorical sampling. It compares all probabilities against one shared random threshold.predict()uses it, which adds a lot of noise (RMSE 2.49 sampled vs. 1.67 deterministic).- The default learning rate is effectively
0.004/60 ≈ 0.00007. At that rate the training RMSE does not decrease at all.
3. What we would need to change to get the paper's numbers
- Replace the visible layer with the paper's one-hot softmax units: weights of shape
(n_items, n_ratings, n_hidden)and biases of shape(n_items, n_ratings). - Use real categorical sampling (
torch.multinomial) during training, and a deterministic expected rating (Σ rating · probability) forpredict(). - Fix the training defaults: remove the
learning_rate / minibatch_sizedivision (or raise the default), and train until the training RMSE converges. - Evaluate the way the paper does: input
Xtr, measure RMSE on the held-out ratings. Target on ML100k: ≈ 0.95, and at minimum beat the item-mean baseline (1.03). If we also report ranking metrics, the input must beXtr, neverXtst.
Reference implementations to compare against: the paper (https://www.cs.toronto.edu/~rsalakhu/papers/rbmcf.pdf), https://github.com/felipecruz/CFRBM (Theano, ML100k, one-hot units), and https://github.com/erwtokritos/collaborativefiltering-rbm (Java, follows the paper exactly).
One minor leftover from the earlier round: in the metrics cell, please pass the arguments literally in each call instead of the shared args / kwargs variables — same style as https://github.com/recommenders-team/recommenders/blob/main/examples/02_model_collaborative_filtering/sar_deep_dive.ipynb.
Signed-off-by: ds-wook <[email protected]>
|
@miguelgfierro 1. The leak is closedConfirmed your finding first. On the same split, with the old TF-ported code:
I've removed the improvement claim from the PR description. Both notebooks now pass Worth noting: 2. Implementation now matches the paper
3. ResultsEvaluated the way the paper does: input ML100k (600 hidden → 200, 150 epochs,
ML1m (400 hidden, 150 epochs, ~45 s)
Ranking metrics, now with the correct input:
For reference, precision@10 on ML100k went from 0.014 to 0.111 against an honest input. 4. Notebooks and tests
|
Description
Migrate the RBM collaborative-filtering model from TensorFlow 1.x to PyTorch.
recommenders/models/rbm/rbm.py is rewritten as a standalone torch.nn.Module, following the same single-file migration pattern established by the LightGCN port (#2315) — no porting of the TF1 graph/sess.run base classes. The public API is kept unchanged so downstream code and the example notebook require no interface changes: RBM(...) constructor signature, fit(), recommend_k_items(), predict(), save()/load(), and the rmse_train attribute all behave as before.
Key changes:
Parameters (w, bv, bh) become nn.Parameter; the tf.lookup.StaticHashTable rating lookup becomes a registered tensor buffer.
The tf.data input pipeline + sess.run training loop is replaced by torch.randperm minibatching with an Adam optimizer.
Contrastive Divergence: the negative phase (gibbs_sampling) runs under torch.no_grad(), and the free-energy difference F(v) - F(v_k) is backpropagated.
Automatic GPU/CPU device selection; checkpoints are saved/loaded via state_dict (.pt).
Behavior fix: the original TF graph applied dropout at inference time (injecting noise into recommendations). The PyTorch version disables dropout in eval() mode, which improves ranking quality (MovieLens-100k MAP@10 ≈ 0.49 / nDCG@10 ≈ 0.64 vs. ≈ 0.14 / 0.41 previously).
examples/00_quick_start/rbm_movielens.ipynb is updated to import torch instead of tensorflow, refresh the TF-specific narrative to PyTorch, switch checkpoint paths to .pt, and use f-strings. The notebook was executed end-to-end on GPU with all cells passing.
Related Issues
References
Checklist:
git commit -s -m "your commit message".staging branchAND NOT TOmain branch.