Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve SAR performance #914

Merged
merged 2 commits into from
Sep 4, 2019
Merged

Improve SAR performance #914

merged 2 commits into from
Sep 4, 2019

Conversation

zegerius
Copy link
Contributor

@zegerius zegerius commented Sep 4, 2019

Co-authored-by: @Overv

Description

During production implementation of the SAR recommender algorithm, we were facing severe performance issues. This was the result of calculating recommendations for all users in the entire set, despite only requiring recommendations for one user.

The approach that was taken during the initial implementation allowed for easy removal of previously 'seen items'.

We changed the size of the affinity matrix that is used for the calculations, by only keeping the rows for the users in the test set, as follows:

https://github.com/microsoft/recommenders/blob/4c875df2d0b6b3ddad70a49a4353c5de19c28e61/reco_utils/recommender/sar/sar_singlenode.py#L307

To be able to do the same thing with the seen-items matrix, we needed to reshape it to reflect the affinity matrix, as follows:

https://github.com/microsoft/recommenders/blob/4c875df2d0b6b3ddad70a49a4353c5de19c28e61/reco_utils/recommender/sar/sar_singlenode.py#L252-L254

To follow the conventions set in the repo, we also clean up the seen-variable during model fitting.

https://github.com/microsoft/recommenders/blob/4c875df2d0b6b3ddad70a49a4353c5de19c28e61/reco_utils/recommender/sar/sar_singlenode.py#L260-L261

This then allows us to do the following, when the remove_seen arg is True:

https://github.com/microsoft/recommenders/blob/4c875df2d0b6b3ddad70a49a4353c5de19c28e61/reco_utils/recommender/sar/sar_singlenode.py#L314-L316

The resulting matrix then has all previously seen items set to -np.inf. The result gets cleaned up, as follows (not part of our PR):

https://github.com/microsoft/recommenders/blob/4c875df2d0b6b3ddad70a49a4353c5de19c28e61/reco_utils/recommender/sar/sar_singlenode.py#L457-L458

Performance

The performance improvement seen is significant, as summarized in the following table:

remove_seen Original Optimized Improvement
True 3.83 s ± 459 ms 17.5 ms ± 1.06 ms 219x
False 3.42 s ± 468 ms 17.9 ms ± 1.7 ms 191x

This analysis was done on our production dataset of approx. 500K rows.

We have not investigated the difference in training time.

Related Issues

#907

Checklist:

  • I have followed the contribution guidelines and code style for this project.
  • I have added tests covering my contributions.
  • I have updated the documentation accordingly.

@msftclas
Copy link

msftclas commented Sep 4, 2019

CLA assistant check
All CLA requirements met.

Copy link
Collaborator

@gramhagen gramhagen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excellent, awesome diagnosis of the problem and improvement. If you can try the suggested change I think this will make it faster and reduce memory overhead.

seen = temp_df[[self.col_user_id, self.col_item_id]].values
self.seen_items = np.ones(self.user_affinity.shape)
self.seen_items[seen[:, 0], seen[:, 1]] = -np.inf

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we remove seen and self.seen_items altogether and leverage the user_affinity since that is already captured, see code change below

# remove items in the train set so recommended items are always novel
if remove_seen:
logger.info("Removing seen items")
test_scores = test_scores * self.seen_items[user_ids, :]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we can add the reduced user_affinity back in after multiplying with -inf, zeros will stay zero and anything we consider the user has seen will have a non-zero entry which will turn to -inf.

test_scores += self.user_affinity[user_ids, :] * -np.inf

@zegerius
Copy link
Contributor Author

zegerius commented Sep 4, 2019

Thank you @gramhagen for that very elegant improvement. I have incorporate your suggested changes. All tests pass.

Also ran a quick performance comparison and I don't see any difference compared to our initial solution (at least not more than one std dev):
remove_seen = True: 18.2 ms ± 1.95 ms (from 17.5 ms ± 1.06 ms)
remove_seen = False: 20.4 ms ± 2.35 ms (from 17.9 ms ± 1.7 ms)

Copy link
Collaborator

@gramhagen gramhagen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great! thanks for the contribution!

@gramhagen gramhagen merged commit dd1a095 into recommenders-team:staging Sep 4, 2019
yueguoguo pushed a commit that referenced this pull request Sep 9, 2019
* Improve score() performance

Co-authored-by: Overv <[email protected]>

* Simplify remove_seen
@zegerius zegerius mentioned this pull request Sep 10, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants