fix problem when learner has no more points for BalancingLearner #214
Conversation
| # Take the points from the cache | ||
| if index not in self._ask_cache: | ||
| self._ask_cache[index] = learner.ask(n=1, tell_pending=False) | ||
| points, loss_improvements = self._ask_cache[index] |
There was a problem hiding this comment.
here one would need a double break, but because that doesn't exist I make it into a function.
|
Wow! This seems to break a lot of tests, will investigate some time later. |
There was a problem hiding this comment.
I think that there's something I must be missing.
Currently the Learner API does not allow a learner to indicate that it has no more points.
If a learner is asked for n points and it returns any more or less than n points then it is in violation of the API. This means that stuff that relies on learners accurately implementing the API is liable to break in unexpected ways.
I notice that the SequenceLearner actually does violate the API, which is why this "bug" appears in the first place. I would imagine that it is a complete accident that the SequenceLearner happens to work with other adaptive infrastructure (specifically the Runner).
It may indeed be a good idea for learners to be able to indicate that they have no more points, but this is something that needs to be looked at carefully and should not just be implemented in an ad-hoc way.
| self._ask_cache[index] = learner.ask(n=1, tell_pending=False) | ||
| points, loss_improvements = self._ask_cache[index] | ||
| if not points: # cannot ask for more points | ||
| return to_select |
There was a problem hiding this comment.
There are a couple of things I don't understand here:
- seems this
returnshould be acontinue; just because learnericould not give any more points does not mean that no other learners can give any! - I cannot see when this branch will ever be executed.
learner.ask(1)is guaranteed to return a point. At the moment there is no way for a learner to indicate that it has "no more points". If a learner returns no points then it is in violation of the API and other stuff is liable to break
Indeed, this is #88 |
|
Seems to me that this should be closed, and we should refer to #88. @basnijholt @akhmerov upvote if you agree |
Closes #213.