ItemNet tests and docs #202
Conversation
…removed filterignores from tests.
|
|
||
| @pytest.mark.parametrize( | ||
| "n_items,n_factors", | ||
| ( |
There was a problem hiding this comment.
Seems we can put it into a single row
| @@ -126,6 +169,11 @@ def from_dataset(cls, dataset: Dataset, n_factors: int, dropout_rate: float) -> | |||
| raise ValueError("`item_features` in `dataset` must be `SparseFeatures` instance.") | |||
|
|
|||
| item_cat_features = item_features.get_cat_features() | |||
|
|
|||
| if item_cat_features.values.size == 0: | |||
There was a problem hiding this comment.
I don't think it's a good idea. We will have this block enabled by default. If dataset doesn't contain categorical features then this embeddings will not be created. But when these features are present they will be used.
This behaviour is the same as for all of our models that work with features (like ALS and Lightfm)
| actual_embedding_dim = cat_item_embeddings.category_embeddings.embedding_dim | ||
|
|
||
| expected_item_features = dataset_item_features.item_features | ||
| # TODO: remove after adding Dense Features support |
There was a problem hiding this comment.
Dense features do not support categories. So there will be no support for CatFeaturesItemNet if features are dense.
@feldlime please correct me if I'm wrong
| expected_item_ids = cat_item_embeddings(items) | ||
| assert expected_item_ids.shape == (n_items, n_factors) | ||
|
|
||
| def test_raises_when_dataset_no_features(self) -> None: |
There was a problem hiding this comment.
This is not an expected behaviour (see other comment). This block (CatFeaturesItemNet) should be ignored if there are:
- No features in dataset
- Features are dense
- No category features are present in sparse features
This block should just be dropped from the item net.
These would be good cases for tests. Model shouldn't raise errors in any of these cases.
| @@ -45,7 +45,7 @@ def get_all_embeddings(self) -> torch.Tensor: | |||
|
|
|||
| @property | |||
| def device(self) -> torch.device: | |||
There was a problem hiding this comment.
I think we should drop all device properties. And stop doing to(device) and do to(<tensor>) instead. Otherwise we can catch problems with lighting multi-device training.
Let's change it to lightning advices for device agnostic code https://pytorch-lightning.readthedocs.io/en/1.0.8/multi_gpu.html
We can do it in this PR or in the next one
Description
Type of change
How Has This Been Tested?
Before submitting a PR, please check yourself against the following list. It would save us quite a lot of time.