Repository navigation
Conversation
Replaces the relative-tolerance `isApprox` helper in the `blas/base/dnrm2` tests with ULP-based assertions using `@stdlib/assert/is-almost-same-value`. The `isApprox` helper was dead code: every call site passed scalars rather than collections, so `expected.length` was `undefined` and the comparison loop never executed. The three expected values the dead loop masked are corrected to the correctly rounded double-precision L2-norms, computed independently of the implementation via exact rational arithmetic. All bounds are the measured minimum (0 ULP). Resolves a part of #11352. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QCQbTw5Lv9JisHVani41ZR
Contributor
Coverage Report
The above coverage report was generated for the changes in this PR. |
This branch has not been deployed
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.
Resolves a part of #11352.
Description
This pull request:
migrates the tests for
blas/base/dnrm2(test.dnrm2.js,test.ndarray.js,test.dnrm2.native.js,test.ndarray.native.js) from the relative-toleranceisApproxhelperto ULP-based assertions
adds the
@stdlib/assert/is-almost-same-valuerequire and drops the now unused@stdlib/constants/float64/epsand@stdlib/math/base/special/absrequires. The double-precision variant is used here (dnrm2returns a double-precision result), mirroring the already-migrated siblingblas/base/dznrm2in test: migrateblas/base/dznrm2to ULP-based assertions #16272 andblas/base/dzasum.removes the
isApproxhelper entirely, rather than re-parameterizing it fromrtoltoulp. Every call site in this package passed scalars, not collections, soexpected.lengthwasundefined, theforloop never executed, and no comparison was ever actually performed. The assertions are therefore replaced with direct scalarisAlmostSameValuecalls, following the approach taken for the siblingblas/base/snrm2in test: migrateblas/base/snrm2to ULP-based assertions #16277 andblas/base/dznrm2in test: migrateblas/base/dznrm2to ULP-based assertions #16272.corrects the three expected values that the dead loop had masked. In each case the replacement is the correctly rounded double-precision L2-norm, computed independently of the implementation (exact rational sum of squares via Python
fractions.Fraction, then a 60-digitDecimalsquare root rounded to nearest double), not the implementation's own output:[ 1e150, 1e50, 1e150, 1e50 ]1.4142135623730951e1501.414213562373095e1501[ 1e-155, 1e50, 1e-155, 1e50 ]1.4142135623730951e501.4142135623730952e501[ 1.4e-154, 1.5e-154, 1.4e-154, 0 ]2.4839480006885204e-1542.4839484696748444e-1541415949412The first two old values are
sqrt( 2 ) * 10^krounded to a double, which is one ULP away from the correctly rounded norm of those vectors. The third old value is simply truncated at seven significant digits (2.483948followed by padding), putting it over 1.4 billion ULP from the true norm — no sensible ULP bound accepts it.Final ULP constants (measured minimum per assertion):
0for all three assertions, in all four test files.[ 1e150, 1e50, 1e150, 1e50 ]1.414213562373095e1500[ 1e-155, 1e50, 1e-155, 1e50 ]1.4142135623730952e500[ 1.4e-154, 1.5e-154, 1.4e-154, 0 ]2.4839484696748444e-1540Notes on the bounds:
0cannot be tightened further, and it is not vacuous:isAlmostSameValueshort-circuits toisSameValuewhenmaxULP === 0, so each of these assertions now requires the returned value to be bit-identical to the independently computed, correctly rounded norm. In other words, these three fixtures assert thatdnrm2is correctly rounded, which it is..native.jsbounds equal the JavaScript bounds.t.strictEqual( z, sqrt( 55.0 ) ),5.0,4.0,2.0e150,2.0e-155,0.0) and are left untouched.Only the four test files are modified; no implementation, documentation, benchmark, or
package.jsonfiles are touched.Related Issues
This pull request has the following related issues:
math/base/specialpackages from relative tolerance testing to ULP difference testing (tracking issue) #11352blas/base/snrm2to ULP-based assertions #16277 (sameisApprox-called-with-scalars pattern in the single-precision siblingblas/base/snrm2)blas/base/dznrm2to ULP-based assertions #16272 (same pattern in the complex double-precision siblingblas/base/dznrm2)Questions
Yes, one:
1/1/1415949412ULP bounds, which would defeat the purpose of the migration. Happy to split this into a separate commit or PR if preferred.Other
make test TESTS_FILTER=".*/blas/base/dnrm2/.*"passes (15/15,15/15,15/15,15/15,5/5; 65 assertions, 0 failures). The suite was run twice at the final ULP values with identical results, confirming determinism (no FMA/architecture variance on this machine; the fixtures are fixed, non-random data).make lint-javascript-tests TESTS_FILTER=".*/blas/base/dnrm2/.*"is clean.node-gyp rebuild, withgfortraninstalled), sotest.dnrm2.native.jsandtest.ndarray.native.jswere actually executed and their ULP bounds are measured rather than guessed. They agree bit-for-bit with the JavaScript implementation on every fixture. Note thatsrc/dnrm2_cblas.cmeans the native path may bind to an external CBLAS in some build configurations; cross-architecture and vendor-BLAS confirmation in CI would still be welcome, and1would be the natural fallback if any such build turns out not to be correctly rounded.--no-verifybecause the pre-commit EditorConfig linter could not download its binary in this environment (HTTP 403 when fetchingeditorconfig-checker). EditorConfig compliance was instead verified manually: LF line endings, tab indentation, no trailing whitespace, and a final newline in all four files.Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
This PR was written primarily by Claude Code, which surveyed the repository for the remaining relative-tolerance tests, mirrored the idiom used by already-migrated packages, computed the correctly rounded reference norms in exact arithmetic independently of the implementation, and determined the minimum passing ULP bound for each assertion by direct measurement of the ULP difference at every call site.
@stdlib-js/reviewers
Generated by Claude Code