Skip to content

Conversation

@amadio
Copy link
Member

@amadio amadio commented Apr 11, 2017

The exact comparison fails in some architectures where rounding may occur.

The exact comparison fails in some architectures where rounding
may occur.
@amadio amadio requested a review from dpiparo April 11, 2017 09:51
@dpiparo
Copy link
Member

dpiparo commented Apr 11, 2017

Hi @amadio, nice pinpointing of the error. I wonder if it wouldn't be better to increase the threshold for 32bits builds only rather than increasing it for 64bits too in order to leave the test as it was where it succeded.

@amadio
Copy link
Member Author

amadio commented Apr 11, 2017

Hi @dpiparo, right above, the test for Point is done like this, so I wouldn't worry about changing the test. It was probably just an oversight. We should avoid using == for floating point as much as possible exactly because of this type of problem.

@dpiparo
Copy link
Member

dpiparo commented Apr 11, 2017

Hi @amadio I understand the point. But a very stringent test was in place for x86_64 and somehow I feel that this gives away some of that rigor.

@peremato
Copy link
Contributor

@dpiparo we did correct other cases similar to this one in other tests with @martinmine. Comparing floating point numbers for equality has to be avoided.

@vgvassilev
Copy link
Member

vgvassilev commented Apr 11, 2017

Just to say that we can use googletests ASSERT_FLOAT_EQ by adding `#include "gtest/gtest.h" and calling ROOT_ADD_GTEST in the cmake file.

@amadio
Copy link
Member Author

amadio commented Apr 11, 2017

I will update this to use the Google test macro as suggested by Vassil.

@martinmine
Copy link
Contributor

@phsft-bot build!

@phsft-bot
Copy link

Starting build on centos7/gcc49, mac1011/native, slc6/gcc49, slc6/gcc62, ubuntu14/native and CMake flags -Dccache=ON -Dimt=OFF

@dpiparo
Copy link
Member

dpiparo commented Apr 11, 2017

@amadio: sure for google test if this does not imply re-writing the entire exec but just reformulating the equality with tolerance.
@mato: with my comment I just wanted to remark that we may loose sensitivity on deviations deriving from factors different from the architecture on x86_64 builds. The 10 epsilons threshold in general is ok for physics performance.

@amadio
Copy link
Member Author

amadio commented Apr 11, 2017

So, since ASSERT_FLOAT_EQ does not return anything, I guess I'd have to fully convert the file to a set of Google tests to use it. I'd rather fix what is broken now and do that in a subsequent PR. What does everyone think?

@pcanal
Copy link
Member

pcanal commented Apr 11, 2017

I'd rather fix what is broken now and do that in a subsequent PR.

@amadio I agree with you.

@dpiparo
Copy link
Member

dpiparo commented Apr 12, 2017

@phsft-bot build!

@phsft-bot
Copy link

Starting build on centos7/gcc49, mac1011/native, slc6/gcc49, slc6/gcc62, ubuntu14/native and CMake flags -Dccache=ON -Dimt=OFF

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants