Skip to content

Conversation

@amadio
Copy link
Member

@amadio amadio commented Apr 13, 2017

Replace direct floating point comparison with AreEqualRel() comparison with a tolerance.

@phsft-bot
Copy link

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

@amadio amadio requested a review from dpiparo April 13, 2017 15:32
@amadio
Copy link
Member Author

amadio commented Apr 13, 2017

My clang-format gives different results than Travis...

for ( unsigned int j = 0; j < ndim; ++j )
{
equals &= fabs(x1[j] - x2[j]) < 1E-15;
if (value1 != value2) return false;
Copy link
Member

@pcanal pcanal Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why keep this equality but replaced the one for error[12]? Aren't both floating points?

Looking at the result, the changes proposed by Travis look correct/improvements.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I though value was an integer from the output of my debug statements, but looking at the interface it is indeed floating point. I will update it.

@pcanal
Copy link
Member

pcanal commented Apr 13, 2017

My clang-format gives different results than Travis...
Are you using clang-format 3.9 (like Travis) or 4.0?

@amadio
Copy link
Member Author

amadio commented Apr 13, 2017

I'm using version 4.0.

@pcanal
Copy link
Member

pcanal commented Apr 13, 2017

How is the result different? If the file that Travis saw was clang-formatted with $ROOTSYS/.clang-format and clang-format 4.0 then there is a regression in clang-format that we need to understand.

@amadio
Copy link
Member Author

amadio commented Apr 13, 2017

Yes, there is probably a bug, or the default configuration for something we don't set manually changed.
I usually just do a git clang-format with each commit hash in my PR as argument and apply all changes until I see no more suggested changes.

@pcanal
Copy link
Member

pcanal commented Apr 13, 2017

or the default configuration for something we don't set manually changed.

We tried to set everything explicit. If you find out what it is, we ought to update the .clang-format.

amadio added 2 commits April 13, 2017 17:52
The test failed because of using x == y comparison with
floating point numbers. This replaces it with a test with
a small tolerance using AreEqualRel() from TMath.
@amadio amadio force-pushed the fix-stress-histogram-test branch from ac575df to 7a35a34 Compare April 13, 2017 15:53
@amadio
Copy link
Member Author

amadio commented Apr 13, 2017

Ok, for now I just manually applied the changes that clang-format wants.

@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 dpiparo merged commit 0f4f032 into root-project:master Apr 13, 2017
@dpiparo
Copy link
Member

dpiparo commented Apr 13, 2017

Thanks @amadio : merged!

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.

4 participants