Skip to content

Conversation

basnijholt
Copy link
Member

This fixes #159.

@akhmerov could you take a look at this?

@basnijholt basnijholt requested a review from akhmerov March 16, 2019 15:27
@basnijholt basnijholt changed the title Distribute points Distribute first points in a BalancingLearner Mar 16, 2019
@akhmerov
Copy link
Contributor

This fix seems to be only partial: consider for example the case when one learner is much cheaper than all the others. Then as soon as one point arrives, the balancing learner will request all points from another random learner in the queue.

If I understand correctly, an empty learner has both an infinite loss and an infinite loss improvement, however some non-empty learners also have the same loss and loss improvement. In that case the problem would arise as well.

Therefore it seems to suggest that a better fix would be to sort on (-loss, n_points) (or loss improvement, depending on the balancing learner setting).

@basnijholt
Copy link
Member Author

@akhmerov, good suggestion!

I've done this in 6e172ef. This is what you meant right?

I'll also do it in _ask_and_tell_based_on_loss.

@akhmerov
Copy link
Contributor

My problem right now is that the logic is rather nontrivial, and it's hard to follow especially due to very similar variable names. Comments might also help.

@basnijholt basnijholt force-pushed the distribute_points branch 3 times, most recently from 8025f97 to 49e73f7 Compare March 17, 2019 10:43
@basnijholt
Copy link
Member Author

I found another issue that I fixed.

I've simplified it now and I think the code is understandable, do you agree?

Copy link
Contributor

@akhmerov akhmerov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@basnijholt basnijholt merged commit 04e7e97 into master Mar 17, 2019
@basnijholt basnijholt deleted the distribute_points branch March 17, 2019 16:58
@basnijholt basnijholt mentioned this pull request Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BalancingLearner puts all points in the first child-learner when asking for points with no data present
2 participants