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: resurrect ActsScalar with float #3754

Closed
wants to merge 15 commits into from

Conversation

AJPfleger
Copy link
Contributor

For some time, we had a rather random usage of ActsScalar instead of double. This generated mismatches child classes which made it impossible to compile the project.

This PR starts to reanimate ActsScalar. For the start we try only a minimal build of Core. We provide:

  • a CI-job, that attempts the minimal build with ACTS_CUSTOM_SCALARTYPE=float, ignoring narrowing warnings
  • fixes for Core, to enable a build

In following PRs, we should aim at also building the unit tests and examples until we can actually run the tests.

@AJPfleger AJPfleger added this to the next milestone Oct 17, 2024
@AJPfleger AJPfleger marked this pull request as draft October 17, 2024 12:57
@AJPfleger
Copy link
Contributor Author

@andiwand Could you maybe help me to adapt the SympyStepper? This is the only failing unit.

[ 53%] Building CXX object Core/CMakeFiles/ActsCore.dir/src/Propagator/SympyStepper.cpp.o
/__w/acts/acts/Core/src/Propagator/SympyStepper.cpp: In member function 'Acts::Result<double> Acts::SympyStepper::stepImpl(State&, Acts::Direction, double, double, std::size_t) const':
/__w/acts/acts/Core/src/Propagator/SympyStepper.cpp:151:12: error: no matching function for call to 'rk4(Eigen::PlainObjectBase<Eigen::Matrix<float, 3, 1> >::Scalar*, Eigen::PlainObjectBase<Eigen::Matrix<float, 3, 1> >::Scalar*, double&, double&, double&, double&, double&, Acts::SympyStepper::stepImpl(State&, Acts::Direction, double, double, std::size_t) const::<lambda(const double*)>&, double*, double, Eigen::MapBase<Eigen::Block<Eigen::Matrix<float, 8, 1, 0, 8, 1>, 3, 1, false>, 1>::ScalarWithConstIfNotLvalue*, Eigen::MapBase<Eigen::Block<Eigen::Matrix<float, 8, 1, 0, 8, 1>, 3, 1, false>, 1>::ScalarWithConstIfNotLvalue*, Eigen::MapBase<Eigen::Block<Eigen::Matrix<float, 8, 1, 0, 8, 1>, 1, 1, false>, 1>::ScalarWithConstIfNotLvalue*, Eigen::PlainObjectBase<Eigen::Matrix<float, 8, 1, 0, 8, 1> >::Scalar*, Eigen::PlainObjectBase<Eigen::Matrix<float, 8, 8, 0, 8, 8> >::Scalar*)'
  151 |         rk4(pos.data(), dir.data(), t, h, qop, m, p_abs, getB, &errorEstimate,
      |         ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  152 |             4 * stepTolerance, state.pars.template segment<3>(eFreePos0).data(),
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  153 |             state.pars.template segment<3>(eFreeDir0).data(),
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  154 |             state.pars.template segment<1>(eFreeTime).data(),
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  155 |             state.derivative.data(),
      |             ~~~~~~~~~~~~~~~~~~~~~~~~
  156 |             state.covTransport ? state.jacTransport.data() : nullptr);
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /__w/acts/acts/Core/src/Propagator/SympyStepper.cpp:17:
/__w/acts/acts/Core/src/Propagator/codegen/sympy_stepper_math.hpp:19:20: note: candidate: 'template<class T, class GetB> Acts::Result<bool> rk4(const T*, const T*, T, T, T, T, T, GetB, T*, T, T*, T*, T*, T*, T*)'
   19 | Acts::Result<bool> rk4(const T* p, const T* d, const T t, const T h,
      |                    ^~~
/__w/acts/acts/Core/src/Propagator/codegen/sympy_stepper_math.hpp:19:20: note:   template argument deduction/substitution failed:
/__w/acts/acts/Core/src/Propagator/SympyStepper.cpp:151:12: note:   deduced conflicting types for parameter 'T' ('float' and 'double')
  151 |         rk4(pos.data(), dir.data(), t, h, qop, m, p_abs, getB, &errorEstimate,
      |         ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  152 |             4 * stepTolerance, state.pars.template segment<3>(eFreePos0).data(),
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  153 |             state.pars.template segment<3>(eFreeDir0).data(),
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  154 |             state.pars.template segment<1>(eFreeTime).data(),
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  155 |             state.derivative.data(),
      |             ~~~~~~~~~~~~~~~~~~~~~~~~
  156 |             state.covTransport ? state.jacTransport.data() : nullptr);
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@andiwand
Copy link
Contributor

@andiwand Could you maybe help me to adapt the SympyStepper? This is the only failing unit.

I think the hardcoded doubles are fighting here with the Vector3 floats. This should be fixable by putting ActsScalar on the offending variables.

On a general note: I wonder if we should rather get rid of ActsScalar because for CPUs it does not make a difference if you use double or float in my experience. I rather observed cases where floats make things worse because of conversions or other CPU black magic.

I think we should use the type which makes sense which is by default double and float in cases someone has a benchmark to proof it is more optimal or for memory reasons like for the material mapping.

@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Oct 17, 2024
@AJPfleger
Copy link
Contributor Author

Thanks, I just read through the code generation and saw, that the relevant part is not generated by Sympy.

Copy link

📊: Physics performance monitoring for 508be93

Full contents

physmon summary

.github/workflows/builds.yml Outdated Show resolved Hide resolved
Core/include/Acts/Geometry/VolumeBounds.hpp Outdated Show resolved Hide resolved
Core/include/Acts/MagneticField/InterpolatedBFieldMap.hpp Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Oct 19, 2024

@paulgessinger
Copy link
Member

Like @andiwand says it might be worth thinking about if we want to retire this typedef entirely at this point, since it doesn't really serve a purpose anymore.

@AJPfleger
Copy link
Contributor Author

We will have a discussion during the workshop next week, how we should proceed on the topic ActsScalar. Currently it seems, that the cost of keeping it outweights the benefits.

@AJPfleger
Copy link
Contributor Author

Let's remove it:

@AJPfleger AJPfleger closed this Nov 19, 2024
@paulgessinger paulgessinger modified the milestones: next, v38.0.0 Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Component - Documentation Affects the documentation Event Data Model Infrastructure Changes to build tools, continous integration, ... Track Finding Track Fitting Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants