Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Geometry/DTGeometryBuilder unit test fails for ARM #29036

Closed
Dr15Jones opened this issue Feb 25, 2020 · 13 comments
Closed

Geometry/DTGeometryBuilder unit test fails for ARM #29036

Dr15Jones opened this issue Feb 25, 2020 · 13 comments

Comments

@Dr15Jones
Copy link
Contributor

The aarch64 IB has the unit tests GeometryDTGeometryBuilderTestDriver failing. The reason is doing a diff on the geometry output is showing an irrelevant difference. One such difference is

< Chamber  Wh:-2 St:1 Se:1  Position  (431.175,39.12,-533.35)  normVect  (-1,1.80411e-16,-6.21021e-20)  bounds W/H/L: 218/36.2/251.1
> Chamber  Wh:-2 St:1 Se:1  Position  (431.175,39.12,-533.35)  normVect  (-1,3.53686e-16,3.02612e-16)  bounds W/H/L: 218/36.2/251.1

where the coordinates of the normal vector which mathematically should be identical to 0. are instead very tiny numbers where x86 and aarch64 disagree on exactly which tiny number to use.

Computationally, any component of a normal vector which is on the order of 10^-7 will be completely irrelevant for bit wise comparisions for calculating the normal and for values produced using a dot product when using floats. For doubles values on the order of 10^-16 will be irrelevant for bitwise comparisons. If we do not care about bit wise, than substantially larger values will be irrelevant for actual physics results.

My suggestion is to either

  1. have the geometry dump code look at the values of the normals and replace anything on the order of 10^-7 with 0. or
  2. have the code that does computes the normal vectors replace values on order 10^-x (where x is some arbirary number > 6) with 0.
@Dr15Jones
Copy link
Contributor Author

assign geometry

@cmsbuild
Copy link
Contributor

New categories assigned: geometry

@Dr15Jones,@cvuosalo,@mdhildreth,@makortel,@ianna,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

A new Issue was created by @Dr15Jones Chris Jones.

@Dr15Jones, @smuzaffar, @silviodonato, @makortel, @davidlange6, @fabiocos can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

@cvuosalo
Copy link
Contributor

I think I will create DataFormats/Math/interface/Rounding.h. It will contain:

namespace rounding {
template <class valType>
inline constexpr valType roundIfCloseTo0(valType value, double tolerance = 1.e-7) {
  if (std::abs(value) < tolerance)
    return (0.0);
  return (value);
}

@cvuosalo
Copy link
Contributor

@Dr15Jones How can I test this issue? I don't know about the ARM machines, and I probably don't have access to them.

@Dr15Jones
Copy link
Contributor Author

I don't know how to access the ARM machines either. I can only look at the results from the related IB.

@ianna
Copy link
Contributor

ianna commented Feb 26, 2020

I think I will create DataFormats/Math/interface/Rounding.h. It will contain:

namespace rounding {
template <class valType>
inline constexpr valType roundIfCloseTo0(valType value, double tolerance = 1.e-7) {
  if (std::abs(value) < tolerance)
    return (0.0);
  return (value);
}

IMHO, this test should produce a reference file dynamically:
https://cmssdt.cern.ch/lxr/source/Geometry/DTGeometryBuilder/test/dtGeometry.log.org

Rounding this normVector will fix the test, but may result in rounding errors down the stream which will change the geometry.

@cvuosalo
Copy link
Contributor

A fix is proposed in PR #29052.

@mrodozov
Copy link
Contributor

mrodozov commented Feb 27, 2020

@ianna , as discussed , one can test a change for arm (or ppc for that matter) adding:

test parameters:
- architecture = slc7_aarch64_gcc820

as a comment, then comment please test, and the bot will take it as default argument (every time, unless the comment is overwritten, edited or deleted)
OR test it specifying the arch like this
please test for slc7_aarch64_gcc820
The access to arm and ppc machines can be requested for a user with CERN techlab if necessary

@ianna
Copy link
Contributor

ianna commented Feb 27, 2020

Fixed via PR #29052

@ianna
Copy link
Contributor

ianna commented Feb 27, 2020

+1

@cmsbuild
Copy link
Contributor

This issue is fully signed and ready to be closed.

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

No branches or pull requests

6 participants