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

feat: SympyStepper #3150

Merged
merged 47 commits into from
May 24, 2024
Merged

feat: SympyStepper #3150

merged 47 commits into from
May 24, 2024

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Apr 27, 2024

An attempt to use generated code instead of Eigen for stepping, jacobians and covariance transport.

@andiwand andiwand added this to the next milestone Apr 27, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module Track Finding labels Apr 27, 2024
@andiwand
Copy link
Contributor Author

andiwand commented Apr 27, 2024

The covariance transport seems to be twice as fast which results in a ~7% speed up of the trackfinding.

The stepping code is not as fast as it could be yet looking at the AtlasStepper performance. This needs a bit more tweaking of the codegen. It looks like the stepping with and without cov can twice as fast as well.

image

image

@asalzburger asalzburger self-requested a review April 29, 2024 09:33
@andiwand
Copy link
Contributor Author

I reverted to using the EigenStepper in the Examples after testing the SympyStepper in the CI. I think for now it makes sense to consistently rely on the EigenStepper.

At some point we should invest some time to properly monitor the different steppers CPU and physics wise. That way we can guarantee that they all work as they should.

The user should be able to transparently swap the EigenStepper out and use the SympyStepper after this PR which should come with some CPU time improvements while physics should stay (mostly up to FP magic) the same.

@github-actions github-actions bot removed the Component - Examples Affects the Examples module label May 23, 2024
@andiwand andiwand requested a review from asalzburger May 23, 2024 09:05
@asalzburger
Copy link
Contributor

Let's go for it.

Copy link

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 53.67232% with 410 lines in your changes missing coverage. Please review.

Project coverage is 47.61%. Comparing base (57fd232) to head (94b2a2f).
Report is 250 commits behind head on main.

Files Patch % Lines
...e/src/Propagator/detail/codegen/sympy_jac_math.hpp 0.00% 242 Missing ⚠️
Core/src/Propagator/codegen/sympy_stepper_math.hpp 80.63% 0 Missing and 61 partials ⚠️
...re/src/Propagator/detail/SympyCovarianceEngine.cpp 20.00% 13 Missing and 31 partials ⚠️
Core/src/Propagator/SympyStepper.cpp 56.84% 5 Missing and 36 partials ⚠️
Core/include/Acts/Propagator/SympyStepper.hpp 73.80% 0 Missing and 11 partials ⚠️
Core/src/Propagator/detail/SympyJacobianEngine.cpp 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3150      +/-   ##
==========================================
+ Coverage   47.42%   47.61%   +0.18%     
==========================================
  Files         499      507       +8     
  Lines       28278    29163     +885     
  Branches    13831    13996     +165     
==========================================
+ Hits        13410    13885     +475     
- Misses       4989     5260     +271     
- Partials     9879    10018     +139     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kodiakhq kodiakhq bot merged commit d7aa303 into acts-project:main May 24, 2024
52 checks passed
@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label May 24, 2024
@andiwand andiwand deleted the tmp-optimized-stepper branch May 25, 2024 06:37
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label May 29, 2024
@paulgessinger
Copy link
Member

As discussed, if we want to rely on this more going forward, we will want to improve the documentation of the generating code.

I would also suggest we try to avoid committing the generated code to the repository.

@andiwand
Copy link
Contributor Author

As discussed, if we want to rely on this more going forward, we will want to improve the documentation of the generating code.

I would also suggest we try to avoid committing the generated code to the repository.

Agreed - Generating the code in a CMake step or pulling it in if this is not possible would be much better. Since it is not a huge amount of code we decided to put it in temporarily. I added a small CI check to see if the generated code is what we expect to avoid this from getting out of sync.

I guess we have a similar problem here https://github.com/acts-project/acts/blob/a48fa8ac4d74f6647c64bb97e14d6d21276636f1/Core/src/Definitions/ParticleDataTable.hpp. Maybe we can find a common solution for this.

Documentations is lacking far behind on this I agree. I will try to put this in as soon as possible

EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 31, 2024
An attempt to use generated code instead of Eigen for stepping, jacobians and covariance transport.
@andiwand andiwand modified the milestones: next, v35.1.0 Jun 1, 2024
Matthewharri pushed a commit to Matthewharri/acts that referenced this pull request Jun 18, 2024
An attempt to use generated code instead of Eigen for stepping, jacobians and covariance transport.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaks Athena build This PR breaks the Athena build Component - Core Affects the Core module Fails Athena tests This PR causes a failure in the Athena tests Infrastructure Changes to build tools, continous integration, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants