Skip to content

test: use epsilon comparison for FMA-sensitive API tests#6591

Merged
kimkulling merged 2 commits intoassimp:masterfrom
pwnorbitals:fix/fp-contract-test-tolerance
Apr 1, 2026
Merged

test: use epsilon comparison for FMA-sensitive API tests#6591
kimkulling merged 2 commits intoassimp:masterfrom
pwnorbitals:fix/fp-contract-test-tolerance

Conversation

@pwnorbitals
Copy link
Copy Markdown
Contributor

@pwnorbitals pwnorbitals commented Mar 27, 2026

Summary

Three API tests fail under -march=znver4 (and potentially other FMA-capable architectures) because GCC's -ffp-contract=fast (the default) makes different FMA contraction decisions for the same inline function compiled in different translation units (shared library vs test binary).

  • aiMatrix3FromToTest and aiMatrix4FromToTest: deterministic failure (fixed input vectors)
  • aiQuaternionFromNormalizedQuaternionTest: non-deterministic (~60% failure rate, random input)

Root cause (verified via disassembly)

The C API wrappers in Assimp.cpp and the direct C++ calls in the test both invoke the same inline functions from .inl headers. Under -march=znver4, the compiler generates different machine code in each TU:

Shared library (FMA chain, 3 roundings):

vfnmadd213ss  1.0, xmm3, xmm1    ; 1.0 - x*x
vfnmadd231ss  xmm2, xmm2, xmm1   ; ... - y*y
vfnmadd231ss  xmm4, xmm4, xmm1   ; ... - z*z

Test binary (separate mul+sub, 6 roundings):

vmulss xmm7, xmm0, xmm0          ; x*x
vmulss xmm6, xmm1, xmm1          ; y*y
vmulss xmm8, xmm2, xmm2          ; z*z
vsubss xmm4, xmm4, xmm8          ; 1.0 - z*z
vsubss xmm4, xmm4, xmm7          ; ... - x*x
vsubss xmm4, xmm4, xmm6          ; ... - y*y

This produces bit-different FP results (1-22 ULPs for matrix elements, branch-flip for quaternion w).

Fix

Replace EXPECT_EQ (bit-exact) with .Equal(epsilon) for the three affected tests. Uses machine epsilon for matrices and 1e-4 for quaternion (larger because the branch in aiQuaternion(aiVector3D) amplifies tiny t differences via sqrt).

Testing

  • All 81 AssimpAPITest_* tests pass under -march=znver4 -O2 (10 repeated runs, 0 failures)
  • All 81 AssimpAPITest_* tests pass under generic x86-64 (no regressions)

Fixes #6246

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Enhanced test assertions with floating-point tolerance comparisons to improve test reliability for numerical operations. Tests now use epsilon-based comparisons instead of strict equality checks, resulting in more robust validation of mathematical computations.

When compiling with -march=znver4 (or any arch with FMA), GCC's default
-ffp-contract=fast contracts a*b+c into FMA opportunistically. The same
inline math function compiled in the shared library and in the test
binary can get different FMA contraction decisions due to different
optimization contexts, producing bit-different FP results.

Three API tests compare C++ direct calls (inlined into test TU) against
C API wrapper calls (through libassimp.so) using EXPECT_EQ (bit-exact),
which fails when the compiler contracts differently across TUs.

Verified via disassembly: the library uses vfnmadd FMA instructions
(3 roundings) while the test binary uses separate vmulss+vsubss
(6 roundings) for the same computation.

Replace EXPECT_EQ with Equal(epsilon) for the three affected tests:
- aiMatrix3FromToTest: use machine epsilon (~1.19e-7)
- aiMatrix4FromToTest: use machine epsilon (~1.19e-7)
- aiQuaternionFromNormalizedQuaternionTest: use 1e-4 because FMA
  differences in 1.0-x*x-y*y-z*z can flip a near-zero residual's sign,
  causing w=0 vs w=sqrt(tiny)≈1e-4

Fixes assimp#6246

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e7842480-b31b-41f4-af6c-d5bda4a5a258

📥 Commits

Reviewing files that changed from the base of the PR and between 8ff0055 and 9dee60a.

📒 Files selected for processing (3)
  • test/unit/AssimpAPITest_aiMatrix3x3.cpp
  • test/unit/AssimpAPITest_aiMatrix4x4.cpp
  • test/unit/AssimpAPITest_aiQuaternion.cpp

📝 Walkthrough

Walkthrough

Three unit test assertions were converted from exact matrix and quaternion equality checks to tolerance-based floating-point comparisons using epsilon values, addressing platform-specific numerical precision variations.

Changes

Cohort / File(s) Summary
Test Tolerance Updates
test/unit/AssimpAPITest_aiMatrix3x3.cpp, test/unit/AssimpAPITest_aiMatrix4x4.cpp, test/unit/AssimpAPITest_aiQuaternion.cpp
Replaced strict equality assertions with epsilon-based floating-point comparisons to accommodate platform-specific numerical precision differences in matrix and quaternion transformations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A rabbit's ode to floating-point grace:

Where numbers dance 'cross platforms wide,
No perfect match need we confide—
With epsilon's gentle, patient sway,
Our tests now pass both night and day! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'test: use epsilon comparison for FMA-sensitive API tests' accurately describes the main change: replacing exact equality checks with epsilon-based floating-point comparisons in three API tests.
Linked Issues check ✅ Passed The changes directly address all coding requirements from issue #6246: the three failing tests (aiMatrix3FromToTest, aiMatrix4FromToTest, aiQuaternionFromNormalizedQuaternionTest) now use epsilon-based comparisons to ensure platform-independent test results.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the three identified failing tests with epsilon-based comparisons, with no unrelated modifications to other tests or code.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Member

@kimkulling kimkulling left a comment

Choose a reason for hiding this comment

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

Looks fine for me.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 1, 2026

@kimkulling kimkulling merged commit 9c10e7d into assimp:master Apr 1, 2026
14 checks passed
@kimkulling
Copy link
Copy Markdown
Member

Merged, thank you for your contribution.

@pwnorbitals
Copy link
Copy Markdown
Contributor Author

Thank you !

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

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

Bug: unittest failures on aarch64-linux

2 participants