Fix TopP sampling dropping the threshold-crossing move - #13
Open
Dash1971 wants to merge 1 commit into
Open
Conversation
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.
When
Temperature > 0andTopP < 1, the sampler currently drops the move that would bring the retained probability mass up to the threshold. For a policy of[0.6, 0.3, 0.1]andTopP=0.8, it samples only the first move, even though that retains just 60% of the probability mass. This PR retains the first two moves and samples them with probabilities[2/3, 1/3].Reproduction
From the repository root, with the normal runtime dependencies installed:
tensor([1.])— only the most likely move can be chosen.tensor([0.6667, 0.3333])— the smallest prefix reaching the threshold is retained and renormalized.The random draw is intercepted only to inspect the exact probabilities supplied to it; softmax, temperature scaling, sorting, filtering, and normalization use the real sampler and PyTorch. No model checkpoint is needed.
Change
The current
cumulative <= top_pmask checks the mass including each candidate and therefore excludes the threshold-crossing candidate. Check the mass before each candidate instead, while always retaining the top move. If a prefix reaches the threshold exactly, no extra move is included. The README now states this behavior explicitly.This can broaden the sampled move distribution for affected settings, including cases that previously collapsed to a single move.
TopP=1andTemperature=0behavior remains unchanged. There are no changes to model weights, inference, or the Elo inputs. The legacycode/uci.pylauncher imports the same implementation, so it benefits from the same fix.Validation
Tested on macOS CPU with Python 3.12.13 and PyTorch 2.12.0. Five of the eleven test methods fail against the unchanged upstream implementation at
1e13597c42d4858b7cfd7cfdae01e297263364b2; all eleven pass with this fix. The distribution tests cover float32 and float64, threshold crossings (including the final move), exact boundaries, the top-one fallback, temperature scaling, original vocabulary indices, masked illegal moves, a single legal move, and unchanged default/argmax behavior. Each expected categorical outcome is checked deterministically rather than relying on random-frequency assertions.