Skip to content

test: migrate stats/base/dists/gamma/logcdf to ULP-based assertions - #16305

Draft
kgryte wants to merge 1 commit into
developfrom
kgryte/ulp-gamma-logcdf
Draft

kgryte wants to merge 1 commit into
developfrom
kgryte/ulp-gamma-logcdf

Conversation

@kgryte

@kgryte kgryte commented Oct 24, 2026

Copy link
Copy Markdown
Member

Resolves a part of #11352.

Description

What is the purpose of this pull request?

This pull request:

  • migrates the tests for stats/base/dists/gamma/logcdf from relative tolerance testing to ULP difference testing.

The package had exactly one relative tolerance comparison, in test/test.native.js:

if ( y === expected ) {
    t.strictEqual( y, expected, 'x: '+x+', alpha: '+alpha+', beta: '+beta+', y: '+y+', expected: '+expected );
} else {
    delta = abs( y - expected );
    tol = 100.0 * EPS * abs( expected );
    t.ok( delta <= tol, 'within tolerance. ...' );
}

which is replaced by

t.strictEqual( isAlmostSameValue( y, expected, 100 ), true, 'returns expected value' );

isAlmostSameValue returns true for exactly equal values, so the y === expected branch collapses into the single assertion. The now-unused abs import and the delta and tol locals are removed; EPS is retained, as it remains in use for generating the random input values.

The existing NOTE explaining the elevated tolerance (differences in accumulated error between the JavaScript and C implementations, as the package wraps cdf, which relies on gammainc, where compiler optimizations can result in different error propagation) is retained, as the rationale still applies to the ULP bound.

ULP bound

The final ULP constant is 100, and this is the one point I would like a reviewer's opinion on.

The measured minimum on this machine is 0. The native addon was built locally and the comparison instrumented with number/float64/base/ulp-difference: across 30 runs of the test (30,000 random input triples), the native C result and ln( cdf( x, alpha, beta ) ) agreed bit-for-bit in every single case. The test also passes with the bound set to 0.

I did not tighten the bound to 0, because doing so would rest entirely on a single platform/compiler. The tolerance branch is effectively dead code on this build, so there is no local measurement that can characterize the JS-vs-C divergence the NOTE describes, and a bound of 0 would be expected to fail on any platform where that divergence is non-zero.

100 was chosen instead as the strictest faithful translation of the previous tolerance. For |expected| in the binade [2^e, 2^(e+1)), one ULP is EPS * 2^e, so 100.0 * EPS * abs( expected ) admitted between 100 and 200 ULP depending on position within the binade. A fixed bound of 100 ULP is therefore never looser than the previous tolerance at any magnitude, while preserving the documented cross-compiler allowance. This is also in line with the bounds used by already-migrated native tests in this family (gamma/cdf uses 131/32/149, normal/logcdf uses 584, gamma/logpdf uses 1024/384/448).

If you would prefer the measured minimum of 0, or a different value, I am happy to change it.

Verification

  • All four test files pass with zero failures: test.js (3), test.factory.js (1030), test.logcdf.js (1026), test.native.js (1026).
  • test.native.js was run twice at the final bound of 100 to confirm determinism; both runs pass 1026/1026.
  • make lint-javascript-files on the changed file reports no new errors. The 8 remaining no-restricted-syntax (FunctionExpression) errors are pre-existing and reproduce identically on the unmodified file from develop.
  • The only file changed is test/test.native.js.

Related Issues

Does this pull request have any related issues?

This pull request has the following related issues:

Reference Packages

Did the content of this pull request follow one or more existing packages? Answering is optional.

This pull request followed these packages:

  • stats/base/dists/normal/logcdf
  • stats/base/dists/gamma/cdf
  • stats/base/dists/gamma/logpdf

What carried over from the reference(s)?

  • The assertion shape t.strictEqual( isAlmostSameValue( y, expected, N ), true, 'returns expected value' ), the 'returns expected value' message, the placement of the @stdlib/assert/is-almost-same-value require directly after tape, and the practice of retaining a generous ULP bound for native JS-vs-C comparisons.

What differed, or what did you have to work out yourself?

  • The referenced packages compare against fixed JSON fixtures, whereas this package generates random inputs via uniform, so there is no fixed fixture set over which to minimize. The bound had to be reasoned about from the previous tolerance's binade-relative width rather than read off a single measured maximum, as described above.

Questions

Any questions for reviewers of this pull request?

Yes — one, on the ULP bound. The measured minimum here is 0, but the previous tolerance and the in-code NOTE both exist to absorb JS-vs-C divergence that does not reproduce on this platform. I kept 100 on the grounds that it is strictly no looser than what the test previously accepted. Please let me know if you would rather have the measured 0.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

test/test.native.js is skipped unless the native addon is present. The addon was built locally with node-gyp rebuild in order to actually exercise the converted assertion; without it, the migrated code path is never run.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

AI Assistance

When authoring the changes proposed in this PR, did you use any kind of AI assistance?

  • Yes
  • No

If you answered "yes" above, how did you use AI assistance?

  • Code generation (e.g., when writing an implementation or fixing a bug)
  • Test/benchmark generation
  • Documentation (including examples)
  • Research and understanding

Disclosure

If you answered "yes" to using AI assistance, please provide a short disclosure indicating how you used AI assistance. This helps reviewers determine how much scrutiny to apply when reviewing your contribution.

This PR was written primarily by Claude Code, running as an unattended scheduled task. It selected the package, studied the already-migrated packages in the same family to match conventions, performed the migration, instrumented the comparison to measure actual ULP differences, and determined the final bound.


@stdlib-js/reviewers


Generated by Claude Code

Replace the relative tolerance comparison in the native test suite with an
ULP difference assertion via `@stdlib/assert/is-almost-same-value`.

The previous tolerance (`100.0 * EPS * abs( expected )`) permitted between
100 and 200 ULP, depending on where the expected value fell within its
binade. The assertion now uses a fixed bound of 100 ULP, which is no looser
than the previous tolerance at any magnitude.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019w9g3A83wPWaSyDCVzKnEb
@stdlib-bot stdlib-bot added Statistics Issue or pull request related to statistical functionality. Good First PR A pull request resolving a Good First Issue. labels Oct 24, 2026
@stdlib-bot

Copy link
Copy Markdown
Contributor

Coverage Report

Package Statements Branches Functions Lines
stats/base/dists/gamma/logcdf $\\color{green}289/289$
$\\color{green}+100.00\\%$
$\\color{green}19/19$
$\\color{green}+100.00\\%$
$\\color{green}4/4$
$\\color{green}+100.00\\%$
$\\color{green}289/289$
$\\color{green}+100.00\\%$

The above coverage report was generated for the changes in this PR.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Good First PR A pull request resolving a Good First Issue. Statistics Issue or pull request related to statistical functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants