-
Notifications
You must be signed in to change notification settings - Fork 42
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
Bandit dispatcher class #372
base: main
Are you sure you want to change the base?
Conversation
f688646
to
3e31b7d
Compare
|
||
|
||
@define | ||
class MultiArmedBanditSurrogate: |
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.
Hey @AVHopp, @Scienfitz: here a first draft of what we've discussed. At this stage, a few comments:
- In principle, this setup works already. However, I'm not yet 100% convinced if this is the best way to go. We should not rush this decision, in particular since the same question (i.e. how to dispatch to more specialized implementations) will soon also arise for our other surrogates, e.g. ApproximateGPs, SparseGPs, ...
- While it works, it does have the disadvantage that you basically loose all autocomplete when you invoke the dispatcher, which is not great. Perhaps we can improve upon this somehow by making it inherit from a (possibly new) base class for all bandit models, where we define the interface? The problem is however, that we cannot inherit from
Surrogate
nor fromSurrogateProtocol
, since the attribute forwarding will get very ugly otherwise - Overall, looking at the code, if we decide to keep it or a similar version, I don't want to expose only the dispatcher. To me, this is more like a utility in the sense that a user has no clue what to use can simply throw their specifications in and will get a reasonable model choice in return. However, there is no reason not to allow also full control by additionally exposing the specialized classes.
What do you think so far? That said, we also don't need to rush with the decision, especially if we agree on the last point. Because than we can also add the dispatcher after the release.
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.
re 2:
would it be possible to create a bandit protocol and inherit from it for that purpose? or does the autocomplete not work with it if its just protocol?
re 3:
I see your argument but it would somehow end up in this situation of us having lots of models laying around. so when I type from baybe.surrogates import
or just search the docs etc about what models are available, I would get lots of models and one model that looks like a base class due to the naming. Even though its not a base class in our case Im always annoyed if packages also expose all the small detailed objects - it will perhaps confuse less senior users.
At the same time, I would argue the dispatcher offers full control
. Eg, for users who know about the beta/bernulli/binary/bandit connection. They simply set of stuff with beta prior and binary target.
Lastly, imo it would be safer to release the special MAB as private, instead of the other way round -> should we reverse the decision no deprecation required
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.
The more I think about it, the more I'd go away again from the strategy design pattern again and rather use a factory approach. This would reduce one level of nesting, make typing/auto-complete more straightforward, and require no additional machinery. Related to that your question about protocols: we already have all we need, I think: there is the Surrogate base class and the SurrogateProtocol, and all specialized bandits are confirm with them. The MAB class on the other hand is NOT a Surrogate (it does not inherit from it and it shouldn't, because I'd then have all stuff twice). And while it sort of complies with the Protocol, it's only because we trick it to do so by forwarding attribute access, causing the typing issues. So I'm really not yet convinced...
While I might also overlook other issues with the alternative approach, it seems a bit clearer to me. It would mean to just replace the MAB class with, say, a factory function make_bandit
that returns an object of type Surrogate
. Done. The downsides are: people need explicitly call the factory, and they need to specify the domain specs upfront and later again. But when regarded only as a utility for automatic model selection as alternative to manually specifying the model, this is completely OK.
So anyway, I have no clear decision at the moment. Your choice to decide what to do 🙃 before you to, perhaps play around with it interactively and experience the feeling and typing
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.
What is the status here now? Should we discuss this in one of our upcoming meetings? Seems like this touches an important, general point that we should align on.
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.
Either that or in a next baybathon. I think it's an important and general design decision we should be very clear about, in case we want to add it. But since it's not pressing, perhaps a baybathon with some other collected topics for it would be nice. Whiteboard sessions are more fun than teams meetings 🙃
searchspace: SearchSpace, objective: Objective | ||
) -> type[BetaBernoulliMultiArmedBanditSurrogate]: | ||
"""Retrieve the appropriate bandit class for the given modelling context.""" | ||
match searchspace, objective: |
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.
included the searchspace in this private (so not affected by deprecation considerations) function why?
|
||
|
||
@define | ||
class MultiArmedBanditSurrogate: |
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.
re 2:
would it be possible to create a bandit protocol and inherit from it for that purpose? or does the autocomplete not work with it if its just protocol?
re 3:
I see your argument but it would somehow end up in this situation of us having lots of models laying around. so when I type from baybe.surrogates import
or just search the docs etc about what models are available, I would get lots of models and one model that looks like a base class due to the naming. Even though its not a base class in our case Im always annoyed if packages also expose all the small detailed objects - it will perhaps confuse less senior users.
At the same time, I would argue the dispatcher offers full control
. Eg, for users who know about the beta/bernulli/binary/bandit connection. They simply set of stuff with beta prior and binary target.
Lastly, imo it would be safer to release the special MAB as private, instead of the other way round -> should we reverse the decision no deprecation required
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.
resetting review, the design apparently is far from over and should be thought through thoroughly because something similar is likely coming for GPs
This PR adds a
MultiArmedBandit
dispatcher class implementing the strategy design pattern.