search

xtensor-stack / xtensor-blas

BLAS extension to xtensor
BSD 3-Clause "New" or "Revised" License
155 stars 54 forks source link

xlapack.solveCholesky test precision too high for 32 bit machines #211

Open drew-parsons opened 2 years ago

drew-parsons commented 2 years ago

The xlapack.solveCholesky test in https://github.com/xtensor-stack/xtensor-blas/blob/0aee2999a3b9edebb58a0ab3ec44eee7f4c5009d/test/test_lapack.cpp#L150 uses an expected x value 0.13757507429403265

This tests fails on 32-bit architectures (or at least i386/i686),

[ RUN      ] xlapack.solveCholesky
/tmp/autopkgtest-lxc.gg3nslld/downtmp/autopkgtest_tmp/test_lapack.cpp:166: Failure
Expected equality of these values:
  x_expected[i]
    Which is: 0.13757507429403265
  x[i]
    Which is: 0.13757507429403248
[  FAILED  ] xlapack.solveCholesky (0 ms)

Full log at https://ci.debian.net/data/autopkgtest/testing/i386/x/xtensor-blas/15978147/log.gz

SylvainCorlay commented 2 years ago

Thanks for the report.

We would be interested in a fix, but presumably it would be interesting to include an i386 test in the CI if we want to catch these.

drew-parsons commented 2 years ago

A pragmatic workaround could be to increase the test tolerance e.g. from 1e-18 to 1e-16 (as an example. I'm not certain what the effective tolerance here is). That could be done here using EXPECT_NEAR in place of EXPECT_DOUBLE_EQ.

SylvainCorlay commented 2 years ago

Thanks! Would you like to open a pull request?

drew-parsons commented 2 years ago

I can do it. I'll need to investigate what tolerance i386 needs. Well, I guess it's just 0.13757507429403265 - 0.13757507429403248 = 2e-16. (the effective tolerance of EXPECT_DOUBLE_EQ must be something like 1e-16)

drew-parsons commented 2 years ago

Also affects armhf, confirming it's a 32-bit issue. https://ci.debian.net/data/autopkgtest/testing/armhf/x/xtensor-blas/15977642/log.gz

drew-parsons commented 2 years ago

armhf is a little coarser, 0.13757507429403265 - 0.13757507429403243 = 2.220446049250313e-16

drew-parsons commented 2 years ago

Looks like 5e-16 is enough. i386 and armhf are both passing now with PR #212, https://ci.debian.net/data/autopkgtest/testing/i386/x/xtensor-blas/15983027/log.gz https://ci.debian.net/data/autopkgtest/testing/armhf/x/xtensor-blas/15982850/log.gz

Eventually test logs will appear at https://ci.debian.net/packages/x/xtensor-blas/