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

fix: compilation problems for Apple Silicon M1 #743

Merged

Conversation

asalzburger
Copy link
Contributor

This PR fixes the compilation on Apple M1 with:

Apple clang version 12.0.0 (clang-1200.0.32.29)
Target: arm64-apple-darwin20.3.0
Thread model: posix

The FloatComparison needed a stricter Enable template definition, as the STL like frontend and the Eigen like frontend could not be resolved.

Also, it revealed another UnitTest in the BoundingBoxTests that is not stable in float (@paulgessinger - we should tackle that, there are still several tests not running, i.e. commented out).

@asalzburger asalzburger self-assigned this Mar 9, 2021
@asalzburger asalzburger changed the title Fix float comparison apple m1 fix: float comparison apple m1 Mar 9, 2021
@asalzburger asalzburger added this to the next milestone Mar 9, 2021
@asalzburger asalzburger added Bug Something isn't working Component - Core Affects the Core module labels Mar 9, 2021
@asalzburger asalzburger changed the title fix: float comparison apple m1 fix: compilation problems for Apple Silicon M1 Mar 9, 2021
@@ -160,7 +160,10 @@ predicate_result matrixCompare(const Eigen::DenseBase<Derived1>& val,
// only accept them. Does someone know a clean way to do that in C++?
//
template <typename Container,
typename Enable = typename Container::const_iterator>
typename CbeginDefined =
std::decay_t<decltype(*cbegin(std::declval<Container>()))>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things surprise me here:

  • I would expect cbegin to be only accessible via std::cbegin, not in isolation.
  • If this is only used for SFINAE, I do not understand why the std::decay_t is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you're right, the std::decay_t should not be used.

For the first point, I will change to *std::cbegin then, I suppose ?

// right on slab
ray3 = {{0, 1, -2}, {0, 0, 1}};
BOOST_CHECK(!bb3.intersect(ray3));
// right on slab - temporarily removed, fails with double precision
Copy link
Contributor

Choose a reason for hiding this comment

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

From the PR description, you probably meant single precision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, actually this is correct, on M1 with double precision, this fails ... all these tests have initially been written by @paulgessinger in float precision, but when introducing ActsScalar a ton of them started to fail due to boundary conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be followed up in an issue & fixed.

@codecov
Copy link

codecov bot commented Mar 9, 2021

Codecov Report

Merging #743 (9124812) into master (7c77035) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #743   +/-   ##
=======================================
  Coverage   48.86%   48.86%           
=======================================
  Files         325      325           
  Lines       16682    16682           
  Branches     7791     7791           
=======================================
  Hits         8151     8151           
  Misses       3061     3061           
  Partials     5470     5470           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c77035...08ac593. Read the comment docs.

@asalzburger asalzburger merged commit a9ec279 into acts-project:master Mar 9, 2021
@paulgessinger paulgessinger modified the milestones: next, v7.0.0 Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Component - Core Affects the Core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants