Repository navigation
Tests: allow last-ulp libm differences in test_velocity_in_ITRF_to_GCRS2 (arm64) - #1147
Open
RegularJoe-CEO wants to merge 1 commit into
Open
RegularJoe-CEO wants to merge 1 commit into
RegularJoe-CEO wants to merge 1 commit into
Conversation
…yfielders#582) test_velocity_in_ITRF_to_GCRS2 divides by a ~7.3e-5 finite-difference motion, so 1-ulp differences in platform sin/cos/atan shift the relative error by ~1.5e-12. On arm64 macOS it measured 1.801e-11 vs the 1.8e-11 limit (x86_64 glibc: 1.735e-11, same NumPy 2.0.2).
There was a problem hiding this comment.
🟢 Approval recommended
The narrowly scoped tolerance increase accommodates the measured numerical variance while preserving a strict accuracy check.
0 open findings
What changed in this PR
Stabilizes a floating-point regression test across arm64 and x86_64 platforms without changing runtime behavior.
Changes:
- Raises the relative-error tolerance from
1.8e-11to2.0e-11. - Documents platform-specific last-ULP variation.
| File | Description |
|---|---|
skyfield/tests/test_positions.py |
Adjusts and explains the cross-platform tolerance. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
The specific failure in #582 (
test_frame_rotation_matricesusing exact==) no longer occurs on master; #844 replaced those checks with tolerances, andtest_planetarylib.pypasses on arm64 macOS today.Running the full suite on current master (31a6ed7) on an Apple M1 (arm64, Python 3.9, NumPy 2.0.2) turns up one remaining arm64-only failure:
Same commit, same NumPy 2.0.2 and Python 3.9 on x86_64 Linux: 1.7352116665064965e-11 (passes, with only ~3.7% headroom).
Root cause: I dumped intermediate values on both machines as hex floats.
t.ttandt.gastare bit-identical. The differences start in the transcendental functions: for 2000 sample inputs,np.sin/math.sindiffer in the last ulp on 90 of them,np.coson 92 andnp.arctanon 41, whilenp.sqrt(correctly rounded under IEEE 754) matches on all 2000. That spreads into 4 elements oft.Mand one component of the rotated position. The test then divides by the motion over one second, about 7.3e-5 of a unit vector, so a 1-ulp change in a position component (~1.1e-16) moves the relative error by ~1.5e-12. That is bigger than the margin the current 1.8e-11 limit leaves.So the computation itself is fine, and the tolerance sits below the floating-point noise floor of the check. This PR raises it to 2.0e-11 and adds a comment explaining why. Results: all 584 tests pass on arm64 macOS and on x86_64 Linux (NumPy 2.0.2).
(Separately, and not touched here: with NumPy 2.5.4 on x86_64 there's an unrelated
UFuncTypeErrorintimelib._utcatwhole += whole2, an int64/float64 in-place add.)This came up during LuxiEdge cross-platform determinism testing.