Repository navigation
refactor: align JS implementation and migrate hypergeometric/variance to ULP-based assertions - #14370
Merged
Merged
Conversation
… assertions Replace computed relative-tolerance comparisons with `isAlmostSameValue` ULP-based assertions in the fixture loops. The JavaScript and C implementations evaluate the same variance formula using different associations, so they require different bounds. The JavaScript implementation computes `n*(K/N)*((N-K)/N)*((N-n)/(N-1))`, whereas the C implementation computes `n*p*(1-p)*((N-n)/(N-1))`, the latter matching the association used by the Julia reference implementation which generated the fixtures. Bounds are the measured minima over the full fixture set: 29 ULP for test.js and 1 ULP for test.native.js. One lower fails in each case. Ref: #11352
stdlib-bot
added
Good First PR
Contributor
Coverage Report
The above coverage report was generated for the changes in this PR. |
kgryte
marked this pull request as ready for review
August 18, 2026 10:58
kgryte
added
Needs Changes
Member
Author
|
We should investigate the divergence between JS and C and determine whether we should align the implementations. |
Compute p=K/N, then n*p*(1-p)*((N-n)/(N-1)) to match the C implementation and the Julia reference. This reduces the maximum ULP deviation from 29 to 1, eliminating the JS/native divergence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
|
/stdlib merge |
stdlib-bot
added
the
bot: In Progress
…geometric-variance
stdlib-bot
removed
the
bot: In Progress
Planeshifter
changed the title
stats/base/dists/hypergeometric/variance to ULP-based assertionshypergeometric/variance to ULP-based assertions
Planeshifter
removed
the
Needs Changes
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
3 participants
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.
Description
This pull request:
p = K/N; n * p * (1-p) * ((N-n)/(N-1))instead ofn * (K/N) * ((N-K)/N) * ((N-n)/(N-1)). The new association matches the Julia reference implementation that generated the test fixtures, bringing the JS ULP bound from 29 down to 1.stats/base/dists/hypergeometric/variancefrom computed relative-tolerance comparisons (delta/tolagainst18.0 * EPS * abs( expected[ i ] )) to ULP-based assertions using@stdlib/assert/is-almost-same-value, per [RFC]: Migratemath/base/specialpackages from relative tolerance testing to ULP difference testing (tracking issue) #11352.@stdlib/math/base/special/absand@stdlib/constants/float64/epsrequires from both test files.ULP bounds
Both implementations now use the same formula association, so both achieve the same tight bound:
test/test.js1test/test.native.js1Related Issues
math/base/specialpackages from relative tolerance testing to ULP difference testing (tracking issue) #11352Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
@stdlib-js/reviewers
🤖 Generated with Claude Code