search

edf-hpc / verrou

floating-point errors checker
https://edf-hpc.github.io/verrou/vr-manual.html
GNU General Public License v2.0
49 stars 13 forks source link

Copy-paste typo in hashRatio? #39

Closed nestordemeure closed 2 years ago

nestordemeure commented 2 years ago

I found what looks like a copy-paste error between the float and double implementations of of hashRatio. Shouldn't this line use 1 / 2**64 since this function deals with 64-bits (double) floating-point numbers?

https://github.com/edf-hpc/verrou/blob/332fe85ad2885c455d99c4ee774a6ac1e14a95b0/interflop_backends/interflop_verrou/dietzfelbingerHash.hxx?fbclid=IwAR03IHV0IE7MVPfU9zcFj6UORbgeatn3gtQNJycil9Fe_0MZOJ7nrQtOowg#L50

lathuili commented 2 years ago

This line is correct as the previous one is a uint32_t thanks the shift >>32. Nevertheless I think it could be possible to replace the previous uint32_t by a uint64_t (without the 32 shift) and replace the invMax by the 64bit version. I do not think it will improve significantly the results (accuracy and performance) but I need to test.

I chose the 32 bit version to keep code similar to the one of multiplyShiftHash but now the two versions are sufficiently different. So now I do no see good reason to keep the 32bit version : I definitively need to do the test.

nestordemeure commented 2 years ago

Ok, that's much clearer now! I think adding a comment to explain why it uses 32bits in the 64bits version of the operation might be worth it (or, indeed, going to 64bits: it should be a quick modification).