issue fix for recommend_from_interactions --exclude doesn't work properly #2333
Conversation
There was a problem hiding this comment.
The change looks good to me. Please also add a unit test for this bug, i.e. a test which fails before your change but passes after change.
|
@TobyRoseman Thanks for your suggestion! Will update unit test and let you know |
There was a problem hiding this comment.
Thanks @dhivyaaxim for adding a unit test. I've requested a few minor changes. Other than that, looks good.
| data=tc.SFrame({'userId':[1,1,1,2,2,2,3,3,3],'movieId':[10,11,12,10,13,14,10,11,14]}) | ||
| exclude_pairs=tc.SFrame({'movieId':[14]}) | ||
| observed_data=tc.SFrame({'movieId':[10]}) | ||
| data_set=tc.item_similarity_recommender.create(data,user_id='userId',item_id='movieId') |
There was a problem hiding this comment.
This is confusing. What you're creating on this line is a model not a data set. I would just call the variable model rather than data_set.
| exclude_pairs=tc.SFrame({'movieId':[14]}) | ||
| observed_data=tc.SFrame({'movieId':[10]}) | ||
| data_set=tc.item_similarity_recommender.create(data,user_id='userId',item_id='movieId') | ||
| observed_data=data_set.recommend_from_interactions(observed_items=observed_data,exclude=exclude_pairs) |
There was a problem hiding this comment.
These are recommendations not observed data. I think recommendations makes a lot more sense than observed_data.
| observed_data=tc.SFrame({'movieId':[10]}) | ||
| data_set=tc.item_similarity_recommender.create(data,user_id='userId',item_id='movieId') | ||
| observed_data=data_set.recommend_from_interactions(observed_items=observed_data,exclude=exclude_pairs) | ||
| assert data['movieId'][8] not in observed_data['movieId'] |
There was a problem hiding this comment.
I think this line would be a lot easier to reason about if it was just assert 14 not in observed_data['movieId'].
|
Thanks @TobyRoseman All changes are addressed ! |
|
Thanks @dhivyaaxim |
This PR resolves #2044
recommend_from_interactions --exclude doesn't work properly