-
Notifications
You must be signed in to change notification settings - Fork 117
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
Add MABSelector
operator selection class to allow using MABWiser to select operators
#153
Conversation
* add docstrings to all MABSelector methods * update MABSelector API: instead of taking a MAB and rebuilding it internally with the correct arm names, expose a function that the MABSelector creator calls to create the arm names as we expect them * properly respect op_coupling in MABSelector
* add tests for MABSelector, still need to add tests for prediction functionality * add validation statements at the beginning of static methods on MABSelector
A few comments right off the bat:
|
@P-bibs hi, thanks for the PR! I'll go through the code changes in a minute. First, in response to various parts of the accompanying text:
I think we can demand this via a protocol class, e.g. from typing import Protocol
class ContextualState(Protocol):
def objective(self) -> float: ...
def get_context(self) -> np.ndarray: ... (I'm guessing the context is a numpy vector)
I think this can get some real estate in the features notebook. Later on we can perhaps add another example where we explicitly compare different selection schemes, but for now I'd go for what's simplest to implement (and that is probably adding it to the features notebook).
No need, I only do this when I make a new release to PyPI. Before then we live in sin for a little while on
I'll squash merge the PR at the end, so this is all good.
I think we can get away with demanding context in general for the
I just fixed that and merged it into this branch. Should be good now! |
Codecov Report
@@ Coverage Diff @@
## master #153 +/- ##
===========================================
- Coverage 100.00% 99.30% -0.70%
===========================================
Files 28 29 +1
Lines 665 719 +54
===========================================
+ Hits 665 714 +49
- Misses 0 5 +5
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Done with my first pass -- this is a lot less code and cleaner integration than I thought it would be! Good job.
There are some last mile items, comments inline. The most high-level ones being readme polishing + usage example + notebook with contextual.
One learning question to @N-Wouda: how did you decide to use "pairs" of destroy, repairs as the operating mechanism --as opposed to keeping the two separate?
Tuples makes it A LOT easier to cast the problem as a Bandit --each tuple becomes an arm. But I am wondering, if they were separate; we could have one Bandit to select over destroy arms and another Bandit to select over repair arms. I am curious if this is just a "syntactical" difference OR there is something more to it in terms of richer structure? faster reward/learning? more expressive/representation power?
alns/select/MABSelector.py
Outdated
self._mab.fit( | ||
[self._operators_to_arm(d_idx, r_idx)], | ||
[self._scores[outcome]], | ||
contexts=self._context_extractor(cand), |
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.
So when we call mab.fit(), if the context_extractor returns None, and things work fine..
@P-bibs BTW, mab objects has a "is_contextual" field. I assume that can come handy
README.md
Outdated
|
||
The mabwiser integration isn't fully documented yet, but you can get started by looking at the example in `examples/tsp_mab.py`. The only difference from the provided `examples/travelling_salesman_problem.ipynb` is the following lines | ||
``` | ||
# USE A MAB ALGORITHM AS A SELECTOR |
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.
We need to make this usage example a bit more concrete. We can only focus on the non-contextual version here in the Readme and assume the standard ALNS parts, like operators, but flash the connection between MAB creation and ALNS calling.
Ideally, it would be great to add a notebook under /examples, say mabwiser_alns.py that has at least two things:
- the full example from readme that actually runs and compiles
- a contextual version (does not have to super meaningful)
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.
This already looks really good, thank you @P-bibs! In my comments I've mostly focused on the high-level interface. I'm not sure if everything I wrote below is possible, so please let me know if something doesn't work the way I thought it did. Definitely let me know if you have a better idea for some of these things!
pyproject.toml
Outdated
@@ -33,6 +33,7 @@ classifiers = [ | |||
python = "^3.8, <4.0" | |||
numpy = ">=1.18.0" | |||
matplotlib = ">=3.5.0" | |||
mabwiser = "^2.7.0" |
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.
Can we add this as an extra
? Something like:
[tool.poetry.dependencies]
python = "^3.8, <4.0"
numpy = ">=1.18.0"
matplotlib = ">=3.5.0"
mabwiser = { version = ">=2.7.0", optional = true }
[tool.poetry.extras]
mabwiser= ["mabwiser"] # optional integration with MABWiser
This way, pip install alns
will only install the existing core dependencies. pip install alns[mabwiser]
then also installs mabwiser
. We can test whether mabwiser
is installed in MABSelector.py
.
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've made the needed changes to pyproject.toml
and also tried my hand at adding code to raise an exception in MABSelector.py
(right before def __init__(...)
. Can you take a look and make sure this is what you had in mind?
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.
Sure! I'm about to hop on a train so I should have some time in a few hours.
alns/select/MABSelector.py
Outdated
def extract_context(state): | ||
if context_extractor is None: | ||
return None | ||
else: | ||
context = context_extractor(state) | ||
if isinstance(context, list): | ||
# if the output is a list, wrap it so it's 2D. Otherwise, | ||
# it's an np array or dataframe, which can be left alone. | ||
context = [context] | ||
return context | ||
|
||
self._context_extractor = extract_context |
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.
With the protocol assumption on ContextualState
, I think we can replace calls of the form self._context_extractor(state)
with state.get_context()
. If MABWiser expects a 2D argument, we can cast the return value to something 2D as np.atleast_2d(state.get_context())
.
alns/select/tests/test_mab.py
Outdated
|
||
|
||
# TODO: | ||
# tests that check the predictions of the mab |
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 think such tests will be really valuable! You might have a look at how we test AlphaUCB
for inspiration.
If I understand @skadio correctly, we can test whether context is needed using the |
We do a bit of both. The older, simpler selection schemes like the roulette wheel mechanisms do exactly what you describe: they first select a destroy operator, and then a repair operator that works well with the selected destroy operator. That works well enough, so it's definitely not a terrible idea. In the context of bandits, the tuple "action space" made more sense to me. This mostly has to do with the rewards. A new candidate solution is produced by the destroy and the repair operator working together, not separately. Assigning the reward for that candidate solution to the operator pair, rather than to each separately, seems to me very logical. Since I want to assign rewards to these operator pairs, the operator pairs naturally have to be the action, rather than each operator separately. Hope that helps! |
I overlooked the reward attribution problem. If we split, then we need to somehow attribute the reward back -- a motivation to keep them together |
Hi all, just a quick update here: I've been away since Tuesday but will be back later today or tomorrow and will implement the fixes then (apologies for the delay!). I'll go through the PR and reply to anything I'm unsure on but other than that consider all proposed changes accepted. |
* Gracefully handle missing mabwiser dependency
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 approach with flags is exactly what I had in mind!
alns/select/MABSelector.py
Outdated
from typing import Callable, List, Optional, Tuple, Union | ||
|
||
import numpy as np | ||
import pandas as pd |
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.
Pandas is not an ALNS dependency - that's why the CI fails. Can we get by not typing that as a possible return value? (open question, I don't know if that's OK for MABWiser)
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.
Could we move this into the conditional import block?
try:
from mabwiser.mab import ...
+ import pandas as pd
except:
...
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.
Yes, I just checked: pandas is a dependency for MABWiser. Let's do what you suggest!
alns/select/MABSelector.py
Outdated
Int. J. Artif. Intell. Tools, 30(4), 2150021:1–2150021:19. | ||
""" | ||
|
||
if not MABWISER_AVAILABLE: |
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 think this needs to go in the init method, not in the class itself. I could be wrong, but I think this gets executed by the parser even when the user is not using MABSelector
.
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.
Darn, I spent a bit puzzling through this and thought this approach might work, but you're correct. The problem with putting this in the constructor is that the type annotations on the constructor arguments get resolved before the function body runs, and this resolution will fail if the user doesn't have MABWiser installed. Could we get away with stringifying the constructor arguments?
def __init__(
...
- learning_policy: LearningPolicy,
+ learning_policy: "LearningPolicy",
...
)
It's unclear to me whether this still results in type checking with current mypy versions.
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 think that's OK for us!
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.
Pushed a fix. I tested with the following three scenarios:
- Don't have mabwiser installed and don't try to use the
MABSelector
(works) - Don't have mabwiser installed and try to use the
MABSelector
(raises an exception) - Have mabwiser installed and try to use the
MABSelector
(works)
I've fixed the conditional import and added some more tests. Serdar and I had previously discussed testing that ALNS's AlphaUCB and MABWiser's UCB1 return the same results since they are implementing the same algorithm, but as far as I can tell this isn't possible in practice since ALNS and MABWiser differ slightly on how they initialize the per-arm mean for each arm. Only one thing left from my perspective: add support for contexts and corresponding tests. |
Hi @P-bibs, I'm a bit under the weather but I'll try to have a look at this sometime this weekend. |
I think it's OK to assume a We should explain this clearly in the docs, both for the API ( Does that help? |
I figured the typing would be an issue here. Good mypy recognises that! I'm OK with the types not quite working out here, and would suggest we add a We could come up with something that type checks nicely, but I feel it might not be worth our time. For a first version I'd rather add a few more tests and make the documentation really clear on what we're doing here and why. If you have something really cool that does work please feel free to showcase that. But if you don't have anything yet, I am not too bothered by this and suggest we spend our time on things that are more valuable in the short run (tests and documentation). |
Sounds good. I will implement that change and make a longer notebook example that fleshes out the usage and motivation behind the MABSelector. |
@N-Wouda I've added tests for contextual mabs as well as a section in the "ALNS Features" example that explains them. From my end, everything is checked off. Would you be able to give a final once-over when you have a chance? |
alns/select/MABSelector.py
Outdated
try: | ||
if self._mab._is_initial_fit: | ||
prediction = self._mab.predict( | ||
contexts=self._context_extractor(curr) | ||
) | ||
return arm2ops(prediction) | ||
except Exception: | ||
else: | ||
# This can happen when the MAB object has not yet been fit on any | ||
# observations. In that case we return any feasible operator index |
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 had to change this to use a conditional instead of exceptions for the control flow. Since the underlying exception raised by MABWiser is simply Exception
and not a specific subclass, the original try/except
block was catching any exception that was thrown while calling is_initial_fit
, which is probably not what we want. (an alternative would be going back to the try/except
method and checking the specific exception string)
I'll have a look at this tomorrow afternoon. Thanks for your hard work so far! |
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.
Looks good to me! A few small things and then I think we can merge this in! 🚀
After those last few things are done (they should be pretty small) I'll merge this in. I'm happy with how it looks. It'll be a valuable addition to ALNS, and I'll try to have a new release out sometime next week! |
Great! Thank you for all your help and taking time to review the changes. It sounds like there may be some research interns working with the library over the summer so we'll hear if there are any major issues soon :) |
Yes, that's the plan! I'm going to merge this in now, check everything locally, and then release a new version to PyPI later today. |
Well done @P-bibs and @skadio! Very nice collaborating on this. And @P-bibs, best of luck with your PhD! |
Fantastic news for the weekend! I will share with team members here, and they can do a first-round user testing next week. Congrats @P-bibs on your very successful senior thesis! Enjoy your summer ahead of the grad school :) |
@N-Wouda very minor comment -- the comments you added in the readme of ALNS (which is great!), is it possible to bundle them under a (sub) header, say # ALNS with MABWiser or sth.. I would like to add a (sub) header in the MABWiser readme, sth like MABWiser for ALNS and then link to the ALNS library and link to "that" (sub)header. That way, we will complete the full circle between both libs to reference users from both sides. BTW, once the team here uses it, we might also consider a social post to spread the word. Similarly, we can talk about it in upcoming events as small extended abstracts or tool sessions etc. something to keep in mind for next conferences |
@skadio does https://github.com/N-Wouda/ALNS#installing-alns provide what you're looking for?
I agree. Let's stay in touch about this over the summer. I'm also curious to hear more about the work of your summer interns! |
Yup, perfect! I can link to that header in mabwiser readme (adding a section on ALNS now) Let me circle back with findings from using the new functionality. |
@skadio great! |
@N-Wouda @P-bibs Quick circle back after more tests. I asked the team to do an external evaluation of our new functionality and to try out more complex MABWiser settings etc. Good news --everything is working expected! And the team was very happy/impressed with the clean integration. It opens the door for many interesting applications/research. The only comment that came, which I thought is worth sharing is: In the quick start example: https://alns.readthedocs.io/en/latest/setup/template.html Potentially, we can add the That would make the context requirement more explicit and the example more complete. Not a big dial, just a comment to share. |
@skadio great, thanks for letting us know. I have just added |
This PR:
Changelog
MABSelector
class that extends theOperatorSelectionScheme
class. Library users can use MABSelector to wrap a multi-armed-bandit object from MABWiser and use it as an operator selection schemeop_coupling
.context_extractor
that takes a function with the signatureState -> np.array
. If a contextual multi-armed-bandit algorithm is used, this argument must be supplied and will be used to extract a context vector from your domain-specific state object.Example Usage
TODO
Bump version in pyproject.toml?rewrite git history to include better messages (or squash)