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: Align overstep limit #2441

Merged
merged 5 commits into from
Sep 14, 2023

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Sep 13, 2023

Overstep limits are defined differently in our 3 steppers. This PR aligns them to the convention in the EigenStepper. The StraightLineStepper returned the wrong sign which might have affected Fatras simulation for neutral particles.

Pulled out of #2336

@andiwand andiwand added this to the next milestone Sep 13, 2023
@github-actions github-actions bot added the Component - Core Affects the Core module label Sep 13, 2023
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #2441 (4a7fa73) into main (e8600a4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2441   +/-   ##
=======================================
  Coverage   49.71%   49.71%           
=======================================
  Files         461      461           
  Lines       25986    25988    +2     
  Branches    11935    11935           
=======================================
+ Hits        12918    12920    +2     
  Misses       4600     4600           
  Partials     8468     8468           
Files Changed Coverage Δ
Core/include/Acts/Propagator/AtlasStepper.hpp 72.22% <100.00%> (+0.03%) ⬆️
...re/include/Acts/Propagator/StraightLineStepper.hpp 73.91% <100.00%> (+0.38%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@andiwand
Copy link
Contributor Author

I looked at the tracksummary and it seems like we reconstruct 4 additional tracks which have some holes and bad pT estimate. I do not think existing performance is impacted but I could double check if necessary

Opinion @paulgessinger @benjaminhuth ?

@benjaminhuth
Copy link
Member

benjaminhuth commented Sep 13, 2023

I looked at the tracksummary and it seems like we reconstruct 4 additional tracks which have some holes and bad pT estimate. I do not think existing performance is impacted but I could double check if necessary

This means, the fitted params in the tracksummary of particles that were reconstructed before don't change significantly, right?
So we can trace the changes back to diff in simulation, that allows more particles to pass the truth seeding cuts or so?

@andiwand
Copy link
Contributor Author

This means, the fitted params in the tracksummary of particles that were reconstructed before don't change significantly, right? So we can trace the changes back to diff in simulation, that allows more particles to pass the truth seeding cuts or so?

This or it was a fit with errors before.

I guess I could also try to diff the particles/hits to see if my hypothesis holds. But I guess a bunch of them also have some slight numerical differences

@paulgessinger
Copy link
Member

The ttbar job is pretty low-stats in any case, so not sure how much we need to read into fluctuations there.

@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Sep 13, 2023
benjaminhuth
benjaminhuth previously approved these changes Sep 14, 2023
@kodiakhq kodiakhq bot merged commit f581704 into acts-project:main Sep 14, 2023
@andiwand andiwand deleted the align-overstep-limit branch September 14, 2023 17:30
@paulgessinger paulgessinger modified the milestones: next, v29.2.0 Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Performance Component - Core Affects the Core module Infrastructure Changes to build tools, continous integration, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants