Skip to content

[LinearAlgebra] Unittests: increase the epsilon for tests with SparseMatrix - #6268

Open
fredroy wants to merge 2 commits into
sofa-framework:masterfrom
fredroy:matrixtest_epsilon_comp
Open

[LinearAlgebra] Unittests: increase the epsilon for tests with SparseMatrix#6268
fredroy wants to merge 2 commits into
sofa-framework:masterfrom
fredroy:matrixtest_epsilon_comp

Conversation

@fredroy

@fredroy fredroy commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Based on

I guess this is more or less the same symptoms as #6267 but in this case, I needed to increase a bit the epsilon to test with.
Note that all the others unit tests do test against 100*epsilon...

Example of output before:

TestSparseMatrices/5.set_fullVec_nrows_reference, where TypeParam = sofa::TestSparseMatricesTraits<double,24u,24u,3u,3u> (0 ms)
[ RUN      ] TestSparseMatrices/5.fullMat_vector_product
The difference between this->vectorMaxDiff(this->fullVec_nrows_reference,this->fullVec_nrows_result) and 0.0 is 1.4210854715202004e-14, which exceeds this->epsilon(), where
this->vectorMaxDiff(this->fullVec_nrows_reference,this->fullVec_nrows_result) evaluates to 1.4210854715202004e-14,
0.0 evaluates to 0, and
this->epsilon() evaluates to 2.2204460492503131e-16.

so at least it makes it consistent 😅

and I used EXPECT_NEAR instead of ASSERT_LT because according to gtest, EXPECT_NEAR (and its siblings) should be used for floating point comparisons.
https://google.github.io/googletest/reference/assertions.html#floating-point


For the record, I asked why we need that (on macos at least)

Why the asymmetry
Clang's default is -ffp-contract=on, which permits fusing a*b + c within one statement. Both loops are eligible; whether the backend fuses depends on how each is lowered:

Mat::operator* has compile-time bounds (24×24) and is fully inlined and unrolled, so each r += m(i,j)*v[j] stays a scalar multiply-add pair the backend fuses into fmla.
FullMatrix::mul has a runtime trip count and a pitch stride. Clang SLP-vectorised the multiplies into fmul.2d but had to keep the reduction sequential (it can't reassociate FP without fast-math), so multiply and add land in separate instructions with no fusion opportunity.
Neither is wrong — C++ explicitly allows contraction, and both results are valid roundings of the same expression.

By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@fredroy fredroy added pr: fix Fix a bug pr: fast merge Minor change that can be merged without waiting for the 7 review days pr: status to review To notify reviewers to review this pull-request labels Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: fast merge Minor change that can be merged without waiting for the 7 review days pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant