Repository navigation
Let parallel_lottery_choices() assign the alternative with id 0 - #83
Merged
Merged
Conversation
The shared array that workers use to see which alternatives have already been claimed was zero-filled, so an alternative with id 0 always looked taken and was never assigned. The array now starts out filled with a -1 sentinel, which the workers filter out when they read it, and uses 64-bit integers so large alternative ids are not truncated. The existing parallel test asserted one fewer unique choice than there are alternatives, which encoded the bug; it now expects every alternative to be filled. The exhausted-alternatives test also uses ids 0 through 4 and asserts that all of them are chosen.
…face worker errors - Alternative ids passed to parallel_lottery_choices() must be non-negative integers, since they travel between workers through a shared integer array; anything else now raises a ValueError before any worker starts. This also rules out an id colliding with the -1 sentinel, which would otherwise have let one alternative be assigned repeatedly. - Omitting chooser_batch_size no longer crashes the single worker on `proc_num * None`; the batch size defaults to the number of choosers. - A worker that exits with an error now raises a RuntimeError in the caller instead of silently returning a partial result. - The existing parallel test makes the alternatives the binding constraint so it actually checks that every alternative gets filled.
The exhausted-alternatives tests used a without-replacement sampler with sample size 3 to provoke a failure on an empty alternatives table, but it also failed whenever a parallel worker saw one or two alternatives left, which happens in a race between workers. They now use the default sampler wrapped in an explicit check for an empty table. The merged-choice-table test asserts that four uniform draws from three alternatives do not all land on the same one, which is false about 1 run in 81; it now fixes the random seed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
parallel_lottery_choices()shares an array of already-chosen alternative ids between the worker processes. The array was allocated zero-filled, and workers treat every value in it as a taken alternative, so an alternative with id 0 always looked taken and was never assigned. With five alternatives numbered 0–4 and plenty of choosers, every run placed four choosers instead of five.Found while writing the regression tests for #75. Code review of the first commit turned up three more silent-failure modes in the same function, fixed here as well.
Fix
-1sentinel, which the workers filter out when they read it, and uses 64-bit integers ('q') instead of 32-bit ('i') so large alternative ids are no longer silently truncated.ValueError, and docstrings are updated. This also rules out an id colliding with the sentinel.chooser_batch_size(the documented default) no longer crashes the worker onproc_num * None. The batch size defaults to the number of choosers.RuntimeErrorin the caller instead of silently returning a partial result.Version
0.3.dev3, with a changelog entry.