- Notifications
You must be signed in to change notification settings - Fork62
fix problem when learner has no more points for BalancingLearner#214
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| # Take the points from the cache | ||
| ifindexnotinself._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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
here one would need a double break, but because that doesn't exist I make it into a function.
basnijholt commentedSep 5, 2019
Wow! This seems to break a lot of tests, will investigate some time later. |
jbweston left a comment
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 there's something I must be missing.
Currently the Learner APIdoes not allow a learner to indicate that it has no more points.
If a learner is asked forn points and it returns any more or less thann 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] | ||
| ifnotpoints:# cannot ask for more points | ||
| returnto_select |
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.
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
akhmerov commentedOct 17, 2019
Indeed, this is#88 |
jbweston commentedNov 21, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Seems to me that this should be closed, and we should refer to#88. @basnijholt@akhmerov upvote if you agree |
Uh oh!
There was an error while loading.Please reload this page.
Closes#213.