Repository navigation
fix(fast): apply n-ary merges and match the reference tie-break exactly - #52
Merged
Merged
Conversation
Two output-changing fixes (closes #47): 1. seq_try_merge assumed minimal merges are always pairs and short-circuited with has_match=false for anything larger, so n>2 merges never applied and BNE's default path re-picked the same merge every step. The guard is now just n < m, matching seq_merge. 2. Ties on max score were broken by largest assembled bytes; the reference (pinned to HuggingFace by test_bpe_matches_huggingface_merges) takes the first tied candidate in traversal order. pick_best now detects ties in the same single pass (no-tie path unchanged), and resolves them with an early-exit walker mirroring emit_merges order; the delta path jumps to the earliest entry via the inverted index. Entries are built in first-occurrence order so scan order equals traversal order. The dead bytes-ranking machinery is removed. New shared test (tests/tokenizers/test_tie_break.py) trains both implementations on a corpus with a genuine 7-way first-merge tie and n=3 merges, asserting hardcoded reference merge lists. fast now produces byte-identical output to the reference on all benchmark cases (BPE 05d1281ec2, BNE e52c6db4bb, Boundless c045c0e773). BPE/Boundless times unchanged; BNE 0.036s -> 0.066s, the honest cost of actually applying its merges (reference: 0.195s). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Closes #47. Both halves, one goal: fast now produces byte-identical output to the reference on every benchmark case.
Fix 1 — n>2 merges never applied (
seq_try_merge)The
(!has_complex_child && only_minimal)short-circuit assumed minimal merges are always pairs, so any merge of size > 2 silently failed to apply throughtry_merge— BNE's default path picked a 4-node merge, applied nothing, and re-picked the same merge every step. The guard is now justn < m, matchingseq_merge's semantics.Fix 2 — tie-break: bytes → traversal order
The reference (pinned to HuggingFace by
test_bpe_matches_huggingface_merges) takes the first tied candidate in traversal order; fast took the largest by assembled bytes. Designed so the common path pays nothing:best_and_tie()finds the max in the same single pass as before and returns immediately when the max is unique — zero extra allocation, which is why BPE/Boundless times are unchanged.first_merge_in) mirrorsemit_mergesorder and short-circuits at the first tied candidate; the delta path uses the existing inverted index to jump straight to the earliest entry containing a tied candidate. (A naive full re-emit per tie regressed Boundless 0.174s → 0.250s; the early exit restored 0.174s.)train_unconn, delta, single, streaming).Output & performance (Zipf bench, wikitext-agnostic digests)
f4bb575b8e05d1281ec205d1281ec2✓59ac6c0fa0(degenerate)e52c6db4bbe52c6db4bb✓5089559be4c045c0e773c045c0e773✓* BNE's old time was the bug: it never applied its merges. 0.066s is the honest cost of correct n-ary training (reference: 0.195s).
Tests
tests/tokenizers/test_tie_break.py: a corpus with a verified 7-way first-merge score tie + n=3 merges, asserting hardcoded reference merge lists — pins the contract for both implementations.With this, the benchmark digest column (#51) converges across implementations — any future divergence is a regression, visible at a glance.
🤖 Generated with Claude Code