project-gemmi / gemmi

macromolecular crystallography library and utilities
https://project-gemmi.github.io/
Mozilla Public License 2.0
205 stars 42 forks source link

Float comparison fails `static_assert(weights[static_cast<int>(El::D)] == 2.0141, "Hmm")` on some architectures #316

Closed merkys closed 1 month ago

merkys commented 1 month ago

When building Coot with Gemmi on i386 the following failure happens:

In file included from /usr/include/gemmi/model.hpp:18,
                 from /usr/include/gemmi/chemcomp_xyz.hpp:12,
                 from /usr/include/gemmi/mmread.hpp:8,
                 from atom-selection-container.cc:38:
/usr/include/gemmi/elem.hpp: In function ‘double gemmi::molecular_weight(El)’:
/usr/include/gemmi/elem.hpp:100:50: error: static assertion failed: Hmm
  100 |   static_assert(weights[static_cast<int>(El::D)] == 2.0141, "Hmm");
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
/usr/include/gemmi/elem.hpp:100:50: note: the comparison reduces to ‘(2.01410000000000000142e+0l == 2.0140999999999999999e+0l)’

To me this seems like a problem with float comparison. Maybe a rounding could be introduced to get past such failures?

wojdyr commented 1 month ago

It's good timing to get it fixed, we are just testing for the next gemmi release.

Any idea why this assertion is triggered when building coot, but now when building gemmi itself?

merkys commented 1 month ago

It's good timing to get it fixed, we are just testing for the next gemmi release.

Happy to hear that.

Any idea why this assertion is triggered when building coot, but now when building gemmi itself?

Good question. Should gemmi's testsuite catch it? Since this arises from an HPP file, I guess the assert is checked only if some compiled C object includes it.

merkys commented 1 month ago

Thanks a lot for fixing this!

wojdyr commented 1 month ago

No problem. I didn't expect that the numbers would be compared as 80-bit long doubles, but the math comprocessor on i386 does everything on 80-bit numbers, so I guess it should be expected.

Still a mystery to me why compiling gemmi worked, the same assertion is checked many times. Anyway, it should be fixed.

pemsley commented 1 month ago

Any idea why this assertion is triggered when building coot, but [not] when building gemmi itself?

Where can we see Debian GEMMI build logs?

My guess is that the problem is related to the host being i386 - I have not built Coot on that for 10 years or more.

Maybe this is related but going the other way?

https://stackoverflow.com/questions/322797/floating-point-precision-when-moving-from-i386-to-x86-64

wojdyr commented 1 month ago

here is a recent build log from Debian: https://buildd.debian.org/status/fetch.php?pkg=gemmi&arch=i386&ver=0.6.5%2Bds-1&stamp=1716290629&raw=0

merkys commented 1 month ago

I think the reason of gemmi build not failing must be the -fexcess-precision=fast C flag which is used by default in Debian builds. If it is present, src/align.cpp compiles, and if not - fails the assert. I do not know why it is present, but -fexcess-precision=fast seems to round off some excess precision.