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

API Refactor #481

Closed
6 tasks done
benfred opened this issue Oct 3, 2021 · 3 comments · Fixed by #494
Closed
6 tasks done

API Refactor #481

benfred opened this issue Oct 3, 2021 · 3 comments · Fixed by #494

Comments

@benfred
Copy link
Owner

benfred commented Oct 3, 2021

The current API could be improved:

  • Return numpy arrays from recommend methods #482
    We currently return recommend results as python lists of tuples of (itemid, score) . We should instead return a numpy array of the itemids, and another numpy array of scores.

  • Change model.fit to take a user_items sparse matrix #484
    model.fit takes an item_users sparse matrix, while model.recommend takes a user_items sparse matrix. It would cause less confusion if both of these methods took a user_items matrix instead. The ALS, LMF and KNN models need both user_items and item_users - so calculate the transpose as the first step, meaning it doesn't really matter which is passed. The BPR model only needs user_items, so would be better to have a user_items matrix.

  • Unify API for rank_items/recommend/recommend_all #489
    We have 3 different methods for recommending items to users : recommend, recommend_all and rank_items. All three of these are inconsistent with the others:

    • recommend_all doesn’t return scores for recommendations, just ranked items - whereas recommend/rank_items do
    • recommend_all throws errors if we would filter out too many items for any user, where rank_items/recommend just return fewer items
    • rank_items doesn’t support filtering users own likes, whereas recommend/recommend_all do
    • recommend has a batch mode operation in recommend_all - but rank_items doesn't have a corresponding batch mode
    • recommend_all only works on the CPU versions of MF models - and isn't included on KNN or GPU models

    I think we should consolidate down to a single method (recommend) which will take either a single userid or a vector of userids to compute recommendations for. We should augment the filtering options, which currently allow you to exclude items, with another parameter (items) which will force only including items on it. This is like adding an item whitelist to the recommend method, to complement the filter_items blacklist parameter we already have - and would make rank_items redundant.

  • Filtering for similar_items and similar_items #488
    The similar_items and similar_users methods don't have the equivalent filtering options that are included in the recommend method. We should also provide the same filtering options for similar_items and similar_users.

  • Approximate nearest neighbour for BPR/LMF and GPU models #487
    Approximate nearest neighbours only works for the ALS model - and not on BPR or LMF. Likewise this doesn't work on the GPU models. We should change the ANN models to be composable - and wrap any other MF model

  • Don't require empty rows in user_items and item_users #526

Where possible we should add deprecation warnings and maintain the legacy API for a couple releases. The first two changes will be unavoidably breaking API changes.

@benfred
Copy link
Owner Author

benfred commented Oct 9, 2021

I've started to push these changes to a branch https://github.com/benfred/implicit/tree/api_refactor

@benfred benfred linked a pull request Nov 26, 2021 that will close this issue
@benfred benfred linked a pull request Nov 26, 2021 that will close this issue
@santiher
Copy link

@benfred it would have been nice to update the version to 1.0.0 following semver given the API change and the backwards incompatible changes.
Took me some time to figure out what was going and why things weren't working.

Thanks for the awesome package!

@benfred
Copy link
Owner Author

benfred commented Apr 23, 2022

@santiher implicit is following semver, but because we are pre-1.0 still - the API can have breaking changes without bumping the major version. According to https://semver.org/#spec-item-4 :

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. 
The public API SHOULD NOT be considered stable.

Sorry for the breakage, I'm hoping to have things mostly stabilized and get ready for a 1.0 later this year

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 a pull request may close this issue.

2 participants