feat(flexmf): add Neural Collaborative Filtering (NCF) model#1093
feat(flexmf): add Neural Collaborative Filtering (NCF) model#1093danhdanhtuan0308 wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1093 +/- ##
==========================================
+ Coverage 89.64% 89.71% +0.06%
==========================================
Files 240 248 +8
Lines 16488 16807 +319
==========================================
+ Hits 14781 15078 +297
- Misses 1707 1729 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mdekstrand
left a comment
mdekstrand
left a comment
There was a problem hiding this comment.
I've got some comments on inheritance and a question about some unfamiliar PyTorch semantics, but there's a higher-level question: would it make the most sense to make this a separate class, or to make it a set of configuration options on the implicit scorer?
Or, alternatively, to make it more fully extend the implicit scorer, and reuse the implicit scorer's model as-is, combining it with a separate model/module implementing the MLP tower, and a third that combines the first two?
| class FlexMFNCFConfig(FlexMFConfigBase): | ||
| """ | ||
| Configuration for NCF (Neural Collaborative Filtering) Scorer. | ||
|
|
||
| Stability: | ||
| Experimental | ||
| """ | ||
|
|
||
| gmf_embedding_size: PositiveInt = 8 | ||
| mlp_embedding_size: PositiveInt = 8 |
There was a problem hiding this comment.
In the current design, this class will inherit embedding_size from FlexMFConfigBase, but then not use it in favor of these two embedding sizes. Is there one of the embedding sizes that it would make sense to set to embedding_size, and then just have a different name for a second one?
There was a problem hiding this comment.
I am thinking adding mlp_layers and embedding_size to FlexMFimplicitConfig and extend FlexMFModel to to optional include the MLP tower which will make one scorer class handles both MF and NCF
| loss: ImplicitLoss = "logistic" | ||
| negative_strategy: NegativeStrategy | None = None | ||
| negative_count: PositiveInt = 4 | ||
| positive_weight: float = 1.0 | ||
|
|
||
| user_bias: bool | None = False | ||
| item_bias: bool = False | ||
| convolution_layers: int = 0 | ||
|
|
||
| def selected_negative_strategy(self) -> NegativeStrategy: | ||
| if self.negative_strategy is not None: | ||
| return self.negative_strategy | ||
| elif self.loss == "warp": | ||
| return "misranked" | ||
| else: | ||
| return "uniform" |
There was a problem hiding this comment.
This is a lot of copied code from the implicit model. Is there a reasonable way to enable more reuse?
(Also see the top-level review comment, though — that might be a cleaner solution to a whole set of problems.)
There was a problem hiding this comment.
Sure I will change this
| out = torch.cat([gmf_out, mlp_out], dim=-1) | ||
| score = self.prediction(out).squeeze(-1) |
There was a problem hiding this comment.
It concatenates the GMF output and MLP outputs to produce a single score per user-item pair.
| return "uniform" | ||
|
|
||
|
|
||
| class FlexMFNCFModel(FlexMFModel): |
There was a problem hiding this comment.
Similar comment on inheritance as above - we're inheriting FlexMFModel, but then the attributes of this class are not a superset of the attributes of the base class. As a general rule, when we are inheriting a class, the derived class should extend the base class, not silently remove parts of it.
2 participants
No description provided.