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

refactor: Unconditionally use blocked mult in EigenStepper #3009

Merged

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Mar 5, 2024

Just to try if this also returns clean outputs, I'm downgrading the if added in #1181 to an assert. Let's see what the CI says here.

just to try if this also returns clean outputs
@paulgessinger paulgessinger added this to the next milestone Mar 5, 2024
@github-actions github-actions bot added the Component - Core Affects the Core module label Mar 5, 2024
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 48.68%. Comparing base (2664480) to head (943fc2c).

Files Patch % Lines
Core/include/Acts/Propagator/EigenStepper.ipp 0.00% 0 Missing and 10 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3009   +/-   ##
=======================================
  Coverage   48.67%   48.68%           
=======================================
  Files         493      493           
  Lines       29055    29052    -3     
  Branches    13853    13850    -3     
=======================================
  Hits        14143    14143           
+ Misses       4966     4965    -1     
+ Partials     9946     9944    -2     

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

@paulgessinger
Copy link
Member Author

All green, no output changes at all it seems.

Should we do this @stephenswat @andiwand ?

@stephenswat
Copy link
Member

Nice!

@andiwand do you have any performance numbers for this?

@paulgessinger
Copy link
Member Author

@stephenswat I can run a quick test.

@paulgessinger
Copy link
Member Author

paulgessinger commented Mar 5, 2024

This is from 10 runs of ODD full chain without IO:

image

Looks pretty promising, I'm rerunning with a bit more stats.

andiwand
andiwand previously approved these changes Mar 5, 2024
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

very interesting! I thought the assert would fire

Core/include/Acts/Propagator/EigenStepper.ipp Outdated Show resolved Hide resolved
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Mar 5, 2024
@paulgessinger
Copy link
Member Author

paulgessinger commented Mar 6, 2024

image

More stats seem to confirm the measurement from before, I think we're looking at about a 5% speedup.

@paulgessinger paulgessinger marked this pull request as ready for review March 6, 2024 08:58
@paulgessinger
Copy link
Member Author

Athena is green now, should we get this in? @andiwand

@paulgessinger paulgessinger removed the Fails Athena tests This PR causes a failure in the Athena tests label Mar 6, 2024
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

lets go

@kodiakhq kodiakhq bot merged commit f25a727 into acts-project:main Mar 6, 2024
57 checks passed
@github-actions github-actions bot removed the automerge label Mar 6, 2024
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Mar 7, 2024
…ect#3009)

Just to try if this also returns clean outputs, I'm downgrading the `if` added in acts-project#1181 to an `assert`. Let's see what the CI says here.
@paulgessinger paulgessinger modified the milestones: next, v33.1.0 Mar 26, 2024
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…ect#3009)

Just to try if this also returns clean outputs, I'm downgrading the `if` added in acts-project#1181 to an `assert`. Let's see what the CI says here.
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
…ect#3009)

Just to try if this also returns clean outputs, I'm downgrading the `if` added in acts-project#1181 to an `assert`. Let's see what the CI says here.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants