Relax NaN comparison in compression tests#2388
Relax NaN comparison in compression tests#2388cary-ilm wants to merge 2 commits intoAcademySoftwareFoundation:mainfrom
Conversation
`compareExact(float)` currently requires bitwise equality, even for NaN, but C vs C++ EXR can generate NaNs that are not bit-wise identical, leading to reports of failures on various architectures. This fixes the failures noted in AcademySoftwareFoundation#1281, AcademySoftwareFoundation#1460, AcademySoftwareFoundation#1628 in the `test*Compression` tests, of the form: F float at 59, 0 not equal: C++ loaded C 0x7fc4e364 (nan) vs C loaded C 7f84e364 (nan) and in `testOptimizedInterleavePatterns`: error reading back channel B pixel 21,-76 got -nan expected -nan This change takes the conservative approach and relaxes the bitwise comparison only for the architectures that are known to have NaN differences. This also changes the method of bitwise comparison of float values. The C++ standard does not guarantee that writing one field of a union and reading another is well-defined, and we've seen cases of it failing with Intel's oneAPI. This changes the comparison to use `memcpy` to a `uint32_t` instead of a union. Made with Cursor Signed-off-by: Cary Phillips <cary@ilm.com>
| return; | ||
| #endif | ||
| uint32_t a,b; memcpy(&a,&af,sizeof a); memcpy(&b,&bf,sizeof b); | ||
| const bool bothNan = std::isnan (af) && std::isnan (bf); |
There was a problem hiding this comment.
It looks like there are two variables, bothNaN and bothNan, with different capitalization. Is that second one ever used?
Would it be too broad just to say 'compareExact' should treat non bit-identical NaNs as identical on all architectures?
I suppose there could be a #define at the top of the file to force bitwise-identical NaN tests for those who want to do their own experiments.
There was a problem hiding this comment.
Oops, that second bothNan was an oversight as a move things around at the last minute. I removed it.
I'd be in favor of losing the #if's completely and relaxing the tests for all architectures. My initial thought was to not relax the test where it's already succeeding, but if we're just going to add in any new platform where the test fails, what's the point?
Is there any real need to ensure that both codepaths produce bit-identical NaNs?
There was a problem hiding this comment.
I think the original intention was that codecs such as ZIP would preserve all bit patterns, so you do get back the same NaN you gave it. This could work on all architectures for some codecs, as long as both the library and the test never actually use float types to handle pixel data, but just treat the data as a bit stream.
It may be safer just to clarify that "lossless" codecs aren't guaranteed to preserve bit patterns of NaN values (Lossless codecs are guaranteed to decompress a NaN back into a NaN, just not necessarily the same one)
Signed-off-by: Cary Phillips <cary@ilm.com>
2 participants
compareExact(float)currently requires bitwise equality, even for NaN, but the C vs C++ codepaths for generating the pixel values can produce NaNs that are not bit-wise identical, leading to reports of failures on various architectures.This fixes the failures noted in #1281, #1460, #1628 in the
test*Compressiontests, of the form:and in
testOptimizedInterleavePatterns:This change takes the conservative approach and relaxes the bitwise comparison only for the architectures that are known to have NaN differences.
This also changes the method of bitwise comparison of float values. The C++ standard does not guarantee that writing one field of a union and reading another is well-defined, and we've seen cases of it failing with Intel's oneAPI. This changes the comparison to use
memcpyto auint32_tinstead of a union.Made with Cursor