Review fixes for PR #354 (TurboQuant): NaN-scoring prepared query, exact shrinkage endpoint, engine-pinned loading factors, corrected docs - #834
Merged
sroussey merged 4 commits intoAug 17, 2026
Conversation
…ndidate `turboPreparedCosineSimilarity` re-checked the three compatibility scalars (bits, seed, dimensions) but nothing about the query's coordinates, while the dot product is bounded by the CANDIDATE's padded length. A `values` shorter than that read past its own end: every such read is `undefined -> NaN`, NaN survives `finishCosine` to become the score, and a ranking sort over NaN is implementation-defined rather than an error — so the failure surfaced as an arbitrary shortlist with nothing reported. Three guards now reject it, placed before the zero early-return so a malformed zero query cannot slip past. Also make the shrinkage map exact at rho = 1. `shrinkageAt` cannot evaluate that point — the `conditionalSd` floor turns the inner conditional expectation into a step function landing on a bin boundary, so Simpson loses ~1.7% of the cross term at 1 bit while `square` stays exact — and the tabulated last knot came out at 0.983377 / 0.989762 / 0.994138 / 0.996838 at 1/2/3/4 bits against a comment claiming g(1) = 1. Every ratio above that knot therefore collapsed to exactly 1. Returning 1 analytically at rho = 1 (the pair is degenerate there: y = x, so E[Q(x)Q(y)] IS E[Q(x)^2]) takes the worst disagreement with the 1-bit Goemans-Williamson closed form from 2.776e-4 to 1.158e-4 on the test's own grid, with the table still strictly increasing at all four widths. No TURBO_QUANTIZE_VERSION bump: the shrinkage map is a scoring-time computation, and the version covers only the codes -> reconstructed-values mapping. Blast radius is bounded — `finishCosine` uses the closed form at 1 bit so no 1-bit score moves, and at 2-4 bits only ratios above the previous last knot move, i.e. true cosines above ~0.9995. Self-similarity still returns exactly 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
`TurboQuantize.ts`'s module doc claims the typed-array encoder "drops into any backend declared at the padded width", and nothing ran it. Three things were asserted only in prose: that `turboQuantizeToTypedArray`'s output is a shape `assertVectorShape` accepts entry by entry (the packed codec deliberately has no storage path, so this is the only turbo encoder that does), that the length is `turboPaddedLength(d)` and not `d` end to end, and that a padded vector still retrieves its neighbour. A regression silently cropping or re-padding, or an `assertVectorShape` change rejecting `Int8Array`, was invisible to every other test in this area. The round trip goes through `putBulk` (which is what runs `validateVectorEntities`) into an `InMemoryVectorStorage` declared at 1024 for a 768-dimensional embedding, then back out of `similaritySearch`. The companion case executes the footgun four separate error messages in this codebase warn about: a store declared at `d` rejects the widened vector on write, which is why `turboPadToPowerOf2` defaults to false. Added to the existing rag test file rather than a new one — it already owns the task's turbo fixtures and timing hooks. The corpus is kept to ~25 vectors because each one is a full workflow run, and it reuses `turboOf` rather than building a second quantization path: the point is that what the TASK emits is storable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
…ublic disclaimer Three documentation defects, each of which sends a reader the wrong way. `turboQuantizedInnerProduct` said "for maximum accuracy, dequantize both sides and take a real dot product". That is the UNCORRECTED estimator below TURBO_MAX_CORRECTED_BITS and reads systematically LOW with nothing reported: measured at d=1024 over 25 Gaussian pairs at true cosine 0.80, against a true mean inner product of 816.8, this helper returns 823.8 while dequantize-then- dot returns 611.2 at 1 bit (-25%), 739.7 at 2 bits (-9%) and 808.7 at 4 bits (-1%). At 5-8 bits the two agree exactly, which is the boundary the constant names. The docstring now says so and points a candidate-set caller at `turboPrepareQuery` + `turboPreparedCosineSimilarity`. The module doc's bias table is captioned "BEFORE the correction and as shipped", but the 1-bit row's "before" column is already Goemans-Williamson- corrected — that correction landed in an earlier commit than the 2-4 bit one. Read as-is the table says 1 bit has no shrinkage to correct, while the code says the raw statistic reads a true 0.80 as 0.59. The caption now names the 2-4 bit correction, the row is marked, and a note gives the genuinely uncorrected 1-bit bias from the closed form: -0.167 at a true 0.50, -0.210 at 0.80, -0.152 at 0.95, roughly 200x the largest number in the table. `VectorQuantizeTask`'s `method` schema description is the one UI-visible surface, and it repeated the "TurboQuant" name with no disclaimer. It now carries the same one the module doc does: the name refers to the borrowed rotation strategy, not to the paper's distribution-fitted level placement, which is not implemented. Renaming the symbols is deliberately NOT done here and is filed as a follow-up. `QuantizationMethod.TURBO`'s "turbo" is a persisted enum value in serialized workflow JSON and in `VectorQuantizeTaskOutput.method`, and `turboSeed` / `turboPadToPowerOf2` are named input ports — so a rename breaks dataflow edges in saved graphs and invalidates cached task outputs. Deprecated aliases cover function names but not port names or the persisted enum without a migration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
… code array `optimalLoadingFactor` solved the two level counts the signed typed-array path reaches (255 for int8, 65535 for int16 — never powers of two, so absent from the table) by ternary search over `quantizerDistortion`, which calls `Math.exp` ~1200 times per evaluation. ECMAScript leaves `Math.exp` implementation- approximated, and this repo runs the same tests under JSC (`bun test`) and V8 (vitest under Node). Measured here the two disagree at 255 levels: 3.9206374677710735810 on Node v22.22.2 against 3.9206374677728756950 on Bun 1.3.11, a relative 4.6e-13. 65535 happens to agree. That is small enough to produce the same bytes today and not small enough to guarantee it — a code is `Math.round((clamped / scale) * max)`, and a value near a half-integer can flip either way — so the golden-bytes test, which asserts an exact sequence derived from this constant, was a weaker pin than it read. Both values are now literals in `SOLVED_LOADING_FACTORS`, consulted via `Object.hasOwn` (a bare index resolves inherited `Object.prototype` keys). The ternary search stays as the fallback for any other level count. Tabulating the exact doubles is preferred to rounding to N significant digits, which would perturb `scale` and could silently change the checked-in bytes. The golden-bytes test gains an INT16 block over the same fixture: the existing int8 block pins the 255 literal, and nothing pinned 65535 at all. Both blocks were run under vitest AND `bun test` and produce identical bytes. Also drop the encoder's boxed `number[]` for a `Uint8Array` — safe by construction, since `assertBits` caps `bits` at 8 so every code is in [0, 255], and `packCodes` reads only `.length` and `codes[i]` (its signature widens to `ArrayLike<number>`). `unpackCodes` already justifies this choice for the read path. Measured peak RSS growth around a single `turboQuantize` at d = 2^20 falls from ~31 MB to ~25 MB, so `MAX_TURBO_DIMENSIONS`'s docstring figure is re-measured rather than left stale. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
sroussey
merged commit Aug 17, 2026
f4d3a6f
into
claude/integrate-arxiv-paper-VF55c
11 checks passed
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.
Addresses the review findings on #354 in four commits. The review found no security issues (all 15 hostile decode shapes throw) and reproduced the maths independently —
GAUSSIAN_LOADING_FACTORSmatches Max (1960) at all eight widths, the shrinkage quadrature matches(2/π)asin(ρ)to 2e-6 over ρ∈[0, 0.97], and the rotation is genuinely orthogonal. What follows is everything else.1 — A prepared query that cannot span its candidate scored NaN, not an error
turboPreparedCosineSimilarityre-checked the three compatibility scalars (bits,seed,dimensions) but nothing about the query's coordinates, while the dot product is bounded by the candidate'spaddedDimensions. Aquery.valuesshorter than that read past its own end. Every such read isundefined → NaN, NaN survivesfinishCosineto become the score — and a rankingsort((a, b) => scores[b] - scores[a])over NaN is implementation-defined, not an error. The failure surfaced as an arbitrarily ordered shortlist with nothing reported at any layer. A throw is strictly better than a plausible-looking wrong answer.Three guards, placed before the
codeNorm === 0early return so a malformed zero query cannot slip past. ThecodeNormcheck is one item beyond the review's literal ask — it is the same defect on the other field, exactly asassertQuantizeResultShapealready argues fornorm, and it cannot break a well-formed caller sinceturboPrepareQueryonly ever emits aMath.sqrtresult or 0.The shrinkage map now reaches exactly 1 at ρ = 1
shrinkageAtcannot evaluate ρ = 1: theconditionalSdfloor turns the inner conditional expectation into a step function landing on a bin boundary, so Simpson loses ~1.7% of the cross term at 1 bit whilesquarestays exact. Measured, the tabulated last knot came out at 0.983377 / 0.989762 / 0.994138 / 0.996838 at 1/2/3/4 bits — against a comment assertingg(1) = 1. Every ratio above that knot had no bracket to interpolate inside and collapsed to exactly ±1, so at 2 bits every pair from |r| ≥ 0.9898 upward scored identically.Returning 1 analytically (the pair is degenerate there —
y = x, soE[Q(x)Q(y)]isE[Q(x)²]) improves the one exact reference this machinery has. SweepinginvertShrinkage(1, ·)againstcos(π(1−r)/2)on the test's own ±200 grid:and the table is still strictly increasing at all four widths.
No
TURBO_QUANTIZE_VERSIONbump. The shrinkage map is a scoring-time computation; the version covers only the codes → reconstructed-values mapping. Blast radius, measured:finishCosineuses the closed form at 1 bit, so no 1-bit score moves; at 2-4 bits only ratios above the previous last knot move — true cosines above ~0.9995. Self-similarity still returns exactly 1, and the bit-for-bit prepared-vs-pairwise test (which usest = 0.999) is preserved by construction since both routes sharefinishCosine.2 — The
IVectorStoragewidth contract was asserted only in proseTurboQuantize.ts's module doc claims the typed-array encoder "drops into any backend declared at the padded width". Nothing ran it. Three things had no test anywhere: thatturboQuantizeToTypedArray's output is a shapeassertVectorShapeaccepts entry by entry (the packed codec deliberately has no storage path, so this is the only turbo encoder that does), that the length isturboPaddedLength(d)and notdend to end, and that a padded vector still retrieves its neighbour.The round trip goes through
putBulk(which is what runsvalidateVectorEntities) into anInMemoryVectorStoragedeclared at 1024 for a 768-dimensional embedding, then back out ofsimilaritySearch. The companion case executes the footgun four separate error messages in this codebase warn about: a store declared atdrejects the widened vector on write, which is precisely whyturboPadToPowerOf2defaults tofalse.Added to the existing rag test file rather than a new one — it already owns the task's turbo fixtures (
turboOf) and timing hooks, so no new license header.3 — Documentation that sent readers the wrong way
turboQuantizedInnerProductsaid "for maximum accuracy, dequantize both sides and take a real dot product". That is the uncorrected estimator belowTURBO_MAX_CORRECTED_BITSand reads systematically low with nothing reported. Measured at d=1024 over 25 Gaussian pairs at true cosine 0.80, against a true mean inner product of 816.8:5-8 bits agree exactly, which is the boundary the constant names.
The module doc's bias table is captioned "BEFORE the correction and as shipped", but the 1-bit row's "before" column is already Goemans-Williamson-corrected — that correction landed in an earlier commit than the 2-4 bit one. Read as-is the table says 1 bit has no shrinkage to correct, while the code says the raw statistic reads a true 0.80 as 0.59. Caption fixed, row marked, and a note gives the genuinely uncorrected 1-bit bias from the closed form: −0.167 at a true 0.50, −0.210 at 0.80, −0.152 at 0.95 — roughly 200x the largest number in the table. The row's measured numbers are left alone; they are honest measurements of the shipped estimator.
VectorQuantizeTask'smethodschema description is the one UI-visible surface and repeated the "TurboQuant" name with no disclaimer. It now carries the same one the module doc does.The rename is deliberately deferred
rotatedScalarQuantizewould touch 16 exported symbols re-exported wholesale byschema-entry.ts, but the blocking reason is worse than breadth:QuantizationMethod.TURBO's"turbo"is a persisted enum value in serialized workflow JSON and inVectorQuantizeTaskOutput.method, andturboSeed/turboPadToPowerOf2are named input ports. Renaming breaks dataflow edges in saved graphs and invalidates cached task outputs (cacheable = true). Deprecated aliases cover function names but not port names or the persisted enum without a migration. Filed as a follow-up next to #798; only the schema disclaimer is done here.4 — Engine-dependent loading factors, and a boxed code array
optimalLoadingFactorsolved the two level counts the signed typed-array path reaches (255 for int8, 65535 for int16 — never powers of two, so absent from the table) by ternary search overquantizerDistortion, which callsMath.exp~1200 times per evaluation. ECMAScript leavesMath.expimplementation-approximated, and this repo runs the same tests under JSC (bun test) and V8 (vitest on Node). Measured here:3.92063746777107358103.92063746777287569505.93823861362581784115.9382386136258178411A relative 4.6e-13 at 255 — small enough to produce the same bytes today, not small enough to guarantee it, since a code is
Math.round((clamped / scale) * max)and a value near a half-integer can flip either way. The golden-bytes test asserts an exact sequence derived from this constant, so it was a weaker pin than it read. Both values are now literals inSOLVED_LOADING_FACTORS, consulted viaObject.hasOwn(a bare index resolves inheritedObject.prototypekeys); the ternary search stays as the fallback. Tabulating the exact doubles is preferred to rounding to N significant digits, which would perturbscaleand could silently change the checked-in bytes.The golden-bytes test gains an INT16 block over the same fixture — the existing int8 block pins the 255 literal, and nothing pinned 65535 at all. Expected values were captured from the failure diff, not hand-computed, and the test was run under both runners producing identical bytes (below).
The encoder's boxed
number[]becomes aUint8Array— safe by construction (assertBitscapsbitsat 8 so every code is in [0, 255];packCodesreads only.lengthandcodes[i], so its signature widens toArrayLike<number>). Re-measured peak RSS growth around a singleturboQuantizeat d = 2²⁰:That is a ~20% move, so
MAX_TURBO_DIMENSIONS's docstring figure is re-measured rather than left stale — leaving it would have been a smaller version of exactly the doc-vs-code defect this PR fixes.Verification
Every named test passes (117/117 across the two touched files):
The golden bytes are identical under both runners — the whole point of the loading-factor pin:
The three suites that must NOT move did not move — the RMSE ceiling, the per-bit-width signed bias bands, and ranking fidelity all pass unchanged (they top out at true 0.95/0.88, below the ~0.9995 where the ρ=1 fix takes effect).
The failures above are all pre-existing and environmental, each verified
This box was running three other sessions' full test suites concurrently, and every failure is a timeout or a runner-API gap:
unit util vitest— 1 failed:TestCredentialPreload.test.tstimed out at 15 s (the file took 162 s under load). Passes 5/5 in isolation. It does not import TurboQuantize.unit util bun— 8 failed: all inWorkerServerBase.race,Base64anddataUri, allTypeError: vi.stubGlobal / vi.unstubAllGlobals / vi.advanceTimersByTimeAsync is not a function— vitest-only APIs Bun's runner does not implement. None of those files were touched by this PR.unit storage vitest— 3 files failed: all PGlite Postgres tests hittingTest timed out in 15000ms; the count varied between two runs of the same command (4 vs 5 tests). Verified by reverting this PR's four files to the base commit (they passed), then restoring them and re-running twice (passed both times, 6/6). This PR touches no storage code.Risks
codeNormguard is the one item beyond the review's literal ask (rationale above; it is the first thing to cut if scope is challenged).Out of scope
The rename; a storage path for the packed codec (#798);
VectorQuantizeTask.quantizeToInt8's L2-then-×127 scaling; re-derivingGAUSSIAN_LOADING_FACTORS/SHRINKAGE_TABLE_POINTS/SHRINKAGE_NODES_PER_UNIT; any version bump.🤖 Generated with Claude Code
https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
Generated by Claude Code