-
Notifications
You must be signed in to change notification settings - Fork 47
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
Refactor recommender signature #220
Conversation
Hi @AVHopp, this PR might come somewhat surprising to you because it is contrary to what we have agreed on, but there have been new developments that require us to prioritize this. Can give you an update in a call 🙃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried my best to wrap my head around this :D See all of my individual comments.
e825e86
to
52bcebb
Compare
Hi @AVHopp, @Scienfitz: have refactored the code quite a bit during rebasing in order to 1) incorporate your comments and 2) improve the design a bit further. This makes many of the existing threads obsolete since the corresponding code lines no longer exist. Will close them and mark with That means basically: let's have another fresh look at the PR 🙃 Mypy problems are now also resolved. There are two remaining open points to discuss for which I'll open separate threads |
52bcebb
to
7b41958
Compare
7118a02
to
88258cb
Compare
88258cb
to
b3c09d1
Compare
An error is already thrown in Bayesian recommender
Otherwise, passing an objective would also be required for non-predictive recommenders, which do not use it
Otherwise, Liskov substitution principle would be violated
b3c09d1
to
c75ad17
Compare
This PR implements a breaking change by refactoring the recommender signature such that:
Objective
Apart from fixing certain responsibilities (e.g.
Campaign
now only acts as a meta-data handler, just like it is supposed to) this enables/prepares several new features:Recommender
s as entry point instead of being forced to go viaCampaign