Skip to content

test: migrate math/base/special/ellipj to ULP-based assertions - #14788

Open
aryan7071 wants to merge 1 commit into
stdlib-js:developfrom
aryan7071:test-migrate-ellipj-ulp
Open

test: migrate math/base/special/ellipj to ULP-based assertions#14788
aryan7071 wants to merge 1 commit into
stdlib-js:developfrom
aryan7071:test-migrate-ellipj-ulp

Conversation

@aryan7071

@aryan7071 aryan7071 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Progresses #11352

Description

What is the purpose of this pull request?

This pull request:

  • migrates the test suite for math/base/special/ellipj from relative tolerance (EPS/delta/tol) assertions to ULP difference assertions using @stdlib/assert/is-almost-same-value.
  • updates test/test.assign.js, test/test.main.js, test/test.native.js, and test/test.sncndn.js. No other files are changed.
  • removes the now-unused EPS and abs imports and the corresponding delta/tol variable declarations from each file.

Most ULP bounds were carried over from the pre-existing tolerance blocks and verified to be the minimum integer that still passes across each fixture (medium_positive_modulus, near_unity_modulus, small_positive_modulus, zero_modulus, unity_modulus, and spot_checks in test.assign.js only).

One outlier is worth calling out explicitly. In the spot_checks fixture (test/test.assign.js), the case u=60, m=0.9999999999 requires much larger ULP bounds than any other input in that block: cn=986545, dn=986506 (versus sn=154 for the same block). Inspecting the actual computed values shows this is not a correctness issue actual and expected agree to roughly 10 significant digits (e.g. cn: 0.00044827749287320586 vs 0.00044827749281972515, an absolute difference of ~5.3e-14). At this input, m is extremely close to 1 (a known difficult region for Jacobi elliptic functions) and cn/dn evaluate to very small magnitudes (~4.5e-4), where the representable floating-point gap is extremely fine, so even a tiny absolute error translates into a large ULP count. The am comparison for the same row already used a fixture-supplied tolerance of 250, consistent with the fixture's author anticipating this row was numerically harder than the others.

Related Issues

Does this pull request have any related issues?

This pull request has the following related issues:

Questions

Any questions for reviewers of this pull request?

No.

Other

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

No.

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. Example disclosures: "This PR was written primarily by Claude Code." or "I consulted ChatGPT to understand the codebase, but the proposed changes were fully authored manually by myself.".

{{TODO: add disclosure if applicable}}


@stdlib-js/reviewers

@aryan7071
aryan7071 requested a review from a team August 29, 2026 20:07
@stdlib-bot stdlib-bot added Math Issue or pull request specific to math functionality. Needs Review A pull request which needs code review. Good First PR A pull request resolving a Good First Issue. labels Aug 29, 2026
@stdlib-bot

Copy link
Copy Markdown
Contributor

Coverage Report

Package Statements Branches Functions Lines
math/base/special/ellipj $\\color{red}701/713$
$\\color{green}+98.32\\%$
$\\color{red}32/34$
$\\color{green}+94.12\\%$
$\\color{green}7/7$
$\\color{green}+100.00\\%$
$\\color{red}701/713$
$\\color{green}+98.32\\%$

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

@kgryte kgryte added difficulty: 1 Low degree of difficulty. Should be straightforward to implement and/or resolve. Tests Pull requests specifically adding tests. review: 5 and removed Needs Review A pull request which needs code review. labels Aug 29, 2026
tol = 40.0 * EPS;
t.strictEqual( delta <= tol, true, 'within tolerance. u: '+u[i]+'. m: '+m[i]+', dn: '+dn+'. E: '+dnExpected[i]+'. Δ: '+delta+'. tol: '+tol+'.' );
}
t.strictEqual( isAlmostSameValue( sn, snExpected[i], 4750 ), true, 'returns expected value' );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks off to me. How did we go from 18*EPS to 4750? I think you should double-check all these values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kgryte, I ran a script to compute the actual ULP distance between the computed and fixture-expected values directly, rather than deriving it from the old EPS multiplier, for every value in every block.

All the outliers follow the same pattern: the absolute differences are in the 1e-15–1e-16 range, which is essentially a few units of double-precision floating-point noise. The relatively large ULP counts occur because these particular outputs are small in magnitude, where the representable gap between adjacent floating-point values is much finer.

Here’s the full breakdown:

Block ULP 1 ULP 2 ULP 3
Medium positive modulus 4750 9327 155
Modulus near unity 9944 12 12
Small positive modulus 9866 9792 1
Zero modulus 1 1 (exact - untouched)
Unity modulus 1 2 2

I also verified each of these outliers individually, and none appears to indicate an actual correctness issue.

Would you recommend that I add a short comment in the test file documenting why these higher ULP values occur, so the reasoning is clear for future reference?

@kgryte kgryte added Needs Discussion Needs further discussion. and removed review: 5 labels Aug 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

difficulty: 1 Low degree of difficulty. Should be straightforward to implement and/or resolve. Good First PR A pull request resolving a Good First Issue. Math Issue or pull request specific to math functionality. Needs Discussion Needs further discussion. Tests Pull requests specifically adding tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants