Repository navigation
fix: WASM add/subtract with a packed histogram operand - #71
Merged
Merged
Conversation
The JS wrapper builds the export name as
`addHistogram${otherSuffix}to${thisSuffix}` (and the `subtract...from...`
twin), but the AssemblyScript module exported the packed-operand variants
as `addPackedHistogramto*` / `subtractPackedHistogramfrom*`. Any
`wasmHistogram.add(wasmPackedHistogram)` therefore threw
"wasm[...] is not a function", whatever the receiver's bucket size.
Rename the ten exports to follow the regular naming scheme and add a test
covering every (receiver, operand) pair of WASM bucket sizes.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lue fix Commit 21b452b changed updatedMaxValue from `value + unitMagnitudeMask` to `value | unitMagnitudeMask` (as in the Java original) but left the AssemblyScript kernel test expecting the old arithmetic, so `npm run astest:ci` has failed on master since then and CI never reached the vitest step. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
alexvictoor
marked this pull request as ready for review
November 8, 2026 17:34
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
WasmHistogram.add/subtractbuild the export name from the two suffixes:addHistogram${other}to${this}andsubtractHistogram${other}from${this}. The AssemblyScript module exported the packed-operand variants under a different scheme (addPackedHistogramto8,subtractPackedHistogramfromPacked, ...), so adding or subtracting a WASM packed histogram to/from any WASM histogram threw:Receiver type does not matter: all five receivers (8, 16, 32, 64, packed) fail when the operand is packed. The reverse direction (packed receiver, flat operand) already worked.
Fix
Rename the ten exports in
assembly/index.tsto the regular scheme (addHistogramPackedto*,subtractHistogramPackedfrom*). No JS change; nothing else referenced the old names.Also: repair the CI build
npm run astest:cihas been failing on master since 21b452b (updatedMaxValuenow usesvalue | unitMagnitudeMaskinstead of+), becauseassembly/__tests__/HistogramCountsKernels.test.tsstill expected the old arithmetic. Since that step runs before vitest, CI never reached the JS tests. The second commit updates the expectation to the OR; the AssemblyScript suite is back to 15277/15277.Tests
New test in
src/Histogram.spec.tsiterating over every (receiver, operand) pair of WASM bucket sizes: add, check total count and max, subtract, check the summary is back to the original. With master's wasm binary the 5 packed-operand cases fail with the error above; with the fix all 25 pass.npm test: 231 passed, 4 skipped.npm run astest:ci: 15277/15277.npm run typecheck: clean.Found while benchmarking for #68 / #69 (the sparse
addbench had no WASM packed row because of this).🤖 Generated with Claude Code