Implicit sequence weights and Implicit factorization weights#122
Implicit sequence weights and Implicit factorization weights#122nikisix wants to merge 21 commits intomaciejkula:masterfrom
Conversation
… factorization models
…ed_loss. rename fit_weighted to _fit_weighted and call it from fit to streamline the user-api. add cscope database files to gitignore.
…ights and removing _fit_weighted.
…tring explaining the masking via sample_weights strategy
maciejkula
left a comment
maciejkula
left a comment
There was a problem hiding this comment.
I think the PR is almost there: there is only one comment where I think the current implementation is incorrect. The rest are suggestions on how I think it could be even better.
In general, I would love to see some tests added. The PR touches quite a few parts of the codebase, and it would be lovely to have some additional assurance that things work.
Ideas for tests:
- Set all weights to zeros: the model should produce random results.
- Set all weights to a very high value: the model should diverge.
- Set all weights for a subset of users to zero: the model should produce random results for those users (but reasonable results for other users).
spotlight/losses.py
Outdated
|
|
||
|
|
||
| def pointwise_loss(positive_predictions, negative_predictions, mask=None): | ||
| def _weighted_loss(loss, sample_weights=None, mask=None): |
There was a problem hiding this comment.
Seeing that we have sample weights, could we make it so that we do not have the mask argument at all? We would just make sure the weights are all zero where the mask is currently zero. This is already accomplished by the implementation you added to the Interactions class: we just need to make sure the weights are always present (if they are None, we just make them all ones before we construct the sequences).
I think that would be a good change because, at the moment, the mask and weight arguments co-exist uncomfortably where it's not clear how they interact --- which you also point out in the docstring.
There was a problem hiding this comment.
Agreed, just hadn't gotten around to it.
There was a problem hiding this comment.
Attempted in most recent commit. Please let me know what you think.
Might a little point in the right direction with your excellent test ideas.
|
Do you have any literature or other references for what's going on in with the transition matrix generator in datasets/synthetic possibly (asking mainly out of curiosity)? |
|
Ok, added most of the tests you outlined. Please let me know what you think. |
|
Mind taking another look at this when you can @maciejkula? |
|
Mind taking another look when you can please? |
3 participants
This branches off of the sample-weights branch we have a pr going on with right now in order to leverage the new losses logic.
I combined the fit and fit_weighted functions for implicit factorizations, subsume mask_padding into sample_weights and create support for sample weights in to_sequence and implicit sequence.
If you're happy with these changes, you can just merge this branch into master and close out the sample-weights PR without merging.