Fix LMF Negative Sampling to Sample Uniformly#747
Fix LMF Negative Sampling to Sample Uniformly#747Fazel94 wants to merge 3 commits intobenfred:mainfrom
Conversation
benfred
left a comment
benfred
left a comment
There was a problem hiding this comment.
thanks for the fixes!
I'll admit I haven't ever looked closely at the LMF code, and I should have reviewed it previously.
I'm pretty positive on most of these changes - but I have my doubts about switching from popularity based negative sampling to uniform sampling, since I've seen that change hurt performance in previous experiments with BPR (where I ended up having to switch to popularity based sampling to match performance for BPR in lightfm). Would it be possible to run a quick experiment to verify that this change helps out performance?
The other fixes look great, and I appreciate the depth that you've dug into this here
| for _ in range(n_factors): | ||
| deriv[_] -= reg * user_vectors[u, _] | ||
| deriv_sum_sq[u, _] += deriv[_] * deriv[_] | ||
| # Sample uniformly from [0, n_items); reject any item the user has |
There was a problem hiding this comment.
I'm unsure about this one change to move to uniform sampling.
When I was originally adding the BPR code here - I noticed that uniformly sampling the negative items performed much worse than sampling based off popularity. The issue seemed to be that because the positive samples are biased towards popularity - sampling uniformly for the negatives produced much weaker negatives.
There was a problem hiding this comment.
It can explain current lack of performance of LMF with respect to ALS and BPR in my tests.
Should I implement popularity based sampling in the PR?
| i = rng.generate(thread_id) | ||
| # indices[indptr[u]:indptr[u+1]] is sorted (guaranteed by fit()), | ||
| # so binary_search gives O(log k) rejection per sample. | ||
| while binary_search(&indices[indptr[u]], &indices[indptr[u + 1]], i): |
There was a problem hiding this comment.
thanks for adding this check here - I had previously noticed in the BPR code that verifying negative samples was essential #103 (comment) , and this should have been in place here too
Bug A: item_vectors.shape[1] returned n_factors+2, not n_items. Fix: use shape[0]. Bug B: RNGVector range was [0, nnz-1] and i = indices[index] only samples from already-interacted items (popularity-biased, never zero-interaction items). Fix: sample i directly from [0, n_items). Bug C: outer negative-sample loop and inner factor loops all used as the loop variable. Each inner loop left _ == n_factors, so the outer loop ran at most once regardless of neg_prop. Fix: use f for inner factor loops. Bug D: a single RNG seeded with nnz-1 was shared by the user-update pass (needs item IDs) and item-update pass (needs user IDs). Fix: two separate RNGVector instances with correct ranges.
Fazel94
force-pushed
the
fix/lmf-negative-sampling
branch
from
721b42e to
31bc5bf
Compare
|
Switched back to popularity-weighted sampling — RNG generates an offset into the global CSR Ran both variants on MovieLens-100k (factors=32, iterations=30, neg_prop=30, same seed):
Popularity wins, consistent with what you saw in BPR. Script is at |
Fazel94
force-pushed
the
fix/lmf-negative-sampling
branch
from
31bc5bf to
bda49d6
Compare
2 participants
Fix four bugs in lmf_update that gutted negative sampling
The negative sampling loop in lmf_update had four bugs that together
meant the model was barely seeing true negatives:
capping the negative loop at ~34 regardless of catalogue size
items — never zero-interaction ones, and popularity-biased on top
_got clobbered by the inner factor loops,so it ran once instead of neg_prop * seen_items times
item-update passes; each needs its own with the correct range
Fix: shape[0] for n_items, sample item/user IDs directly from [0, n)
with rejection for positives, distinct loop variables, two RNGVectors.
Added five regression tests covering each bug and the overall cluster
recovery behavior.
AI use disclosure:
I have used LLMs extensively for understanding the issue, cleaning up and generating comments for my code and pr.
I have written the code and am responsible for it.