Repository navigation
fix: updatedMaxValue inflates maxValue when lowestDiscernibleValue > 1 - #59
Open
alexvictoor wants to merge 1 commit into
Open
alexvictoor wants to merge 1 commit into
alexvictoor wants to merge 1 commit into
Conversation
The Java original tracks the maximum as value | unitMagnitudeMask, setting the low unitMagnitude bits so the stored maximum is the top of the finest resolvable range. Both the TypeScript and AssemblyScript ports did value + unitMagnitudeMask instead, which over-reports maxValue by up to one mask width whenever the low bits are already partly set (e.g. +511 with the event-loop-delay default lowest of 1000). The TypeScript fix is arithmetic (floor-divide, multiply, add mask) because bitwise OR truncates to 32 bits in JavaScript; the AssemblyScript port uses a plain u64 OR. 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.
Problem
The Java original tracks the maximum as
value | unitMagnitudeMask, setting the lowunitMagnitudebits so the stored maximum is the top of the finest resolvable range. Both ports in this repo didvalue + unitMagnitudeMaskinstead:src/JsHistogram.ts(updatedMaxValue)assembly/Histogram.ts(updatedMaxValue)With
lowestDiscernibleValue: 1the mask is 0 and the two agree, so the default path was unaffected. With a largerlowest(e.g. 1000, the event-loop-delay convention, mask = 511), a value whose low bits are already partly set gets inflated by up to one mask width: recording 9,999,000,063 reportedmaxValue9,999,000,574 instead of 9,999,000,063.Only the
maxValuegetter and the percentile-distribution footer are affected; percentiles, mean, stddev and the iterators read the counts array and were always correct.Fix
floor(value / 2^unitMagnitude) * 2^unitMagnitude + mask), since bitwise|truncates to 32 bits in JavaScript and values can exceed that.u64OR.lowest: 1000, covering a value with partially-set low bits and one with the low bits already all set.Tests
npm test: 181 passednpm run astest:ci: 113/113🤖 Generated with Claude Code