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

Implement returning already liked items #41

Closed
wants to merge 3 commits into from

Conversation

vhfmag
Copy link

@vhfmag vhfmag commented Jul 5, 2017

As asked in issue #40

@benfred
Copy link
Owner

benfred commented Jul 6, 2017

Thanks for the PR!

There are a couple minor issues highlighted by the failing travis ci build though : the annoy_als.py version fails to run the unittests (best should be zip(ids, dist)) and there are some style issues from flake8.

For the API, I think the parameter should be filter_liked=True - rather than return_liked=False

Also I think in this case we could be more efficient in how we're returning the items. Each 'recommend' method is querying for count = N + len(liked) items (like here), but in this case we're only need the N items. Maybe we should pull out the code that builds the 'liked' set to a common member on the base class - but only if the filter_liked flag is set. What do you think?

@vhfmag
Copy link
Author

vhfmag commented Jul 7, 2017

I'll work at it this weekend. I thought of implementing as much as possible of the recommendation function in the base class and creating a private abstract method for the algorithm-dependent logic, what do you think?

I'm not familiar with flake8, so I'll read about it before proceding :)

Victor Magalhães added 2 commits July 9, 2017 14:42
Reimplement recommendation base method as a composition of smaller
abstract methods
@vhfmag
Copy link
Author

vhfmag commented Jul 9, 2017

Hello! Did it :)

I tried to put all the recommendation logic in the RecommenderBase by creating two abstract methods: liked and best_recommendations.

Also, there were some style issues with lines being too long, specially function calls or definitions, so I put each parameter in a separated line, I don't know if it's the best practice, but it passed the test.

@vhfmag
Copy link
Author

vhfmag commented Jul 14, 2017

Any updates?

@ghvg1313
Copy link

@benfred Is the only issue being the conflicts here? @vhfmag We are trying to achieve the exact same thing to validate the model. I might be able to help resolve the conflicts.

@lautjy
Copy link

lautjy commented Sep 21, 2018

IMO this should be closed.
This PR implements desired functionality and is in master:
fd984bc

@vhfmag vhfmag closed this Dec 4, 2018
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.

4 participants