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!: Rename Single*TrackParameters to Generic*TrackParameters #2269

Merged

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Jul 4, 2023

I am unhappy with the name Single*TrackParameters as it could refer to singly charged or single track. The track actually supports different kinds of charge hypothesis so I thought Generic*TrackParameter might fit better.

I also merged NeutralTrackParameters into TrackParameters and removed the external template instantiation as it could be a performance hit because it disables inlining.

Pulled these changes out of #2181

@andiwand andiwand added the Component - Core Affects the Core module label Jul 4, 2023
@andiwand andiwand added this to the next milestone Jul 4, 2023
@andiwand
Copy link
Contributor Author

andiwand commented Jul 4, 2023

not sure if we consider this one breaking as the typedefs did not change

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #2269 (6d9627f) into main (1e2eb5d) will increase coverage by 0.08%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##             main    #2269      +/-   ##
==========================================
+ Coverage   49.47%   49.56%   +0.08%     
==========================================
  Files         451      451              
  Lines       25484    25477       -7     
  Branches    11720    11700      -20     
==========================================
+ Hits        12609    12628      +19     
+ Misses       4582     4575       -7     
+ Partials     8293     8274      -19     
Files Changed Coverage Δ
...s/EventData/MultiComponentBoundTrackParameters.hpp 61.81% <ø> (ø)
Core/include/Acts/Propagator/AtlasStepper.hpp 72.26% <ø> (+0.23%) ⬆️
Core/include/Acts/Propagator/EigenStepper.hpp 78.00% <ø> (+4.00%) ⬆️
Core/include/Acts/Propagator/EigenStepper.ipp 53.84% <ø> (ø)
.../include/Acts/Propagator/MultiEigenStepperLoop.hpp 68.64% <ø> (ø)
...re/include/Acts/Propagator/StraightLineStepper.hpp 73.91% <ø> (ø)
Core/src/Material/SurfaceMaterialMapper.cpp 7.88% <ø> (ø)
Core/src/Material/VolumeMaterialMapper.cpp 9.43% <ø> (ø)
Core/src/Propagator/detail/CovarianceEngine.cpp 50.00% <ø> (ø)
...ude/Acts/EventData/GenericBoundTrackParameters.hpp 67.10% <85.71%> (ø)
... and 2 more

... and 7 files with indirect coverage changes

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

@github-actions
Copy link

github-actions bot commented Jul 4, 2023

📊 Physics performance monitoring for 6d9627f

Summary
Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
AMVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

AMVF seeded

AMVF truth_smeared

AMVF truth_estimated

AMVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@andiwand andiwand requested a review from asalzburger July 4, 2023 15:10
@paulgessinger
Copy link
Member

paulgessinger commented Jul 4, 2023

@andiwand The top-level typedefs don't change name or meaning, really. OTOH the other typenames do in fact change. I guess I'd lean towards calling it breaking.

@andiwand andiwand changed the title refactor: Rename Single*TrackParameters to Generic*TrackParameters refactor!: Rename Single*TrackParameters to Generic*TrackParameters Jul 4, 2023
@andiwand
Copy link
Contributor Author

andiwand commented Jul 5, 2023

@paulgessinger should we hold this back until we have all the breaking changes ready for one release? if so, how should we flag that? blocked or should we create a milestone for that?

@andiwand andiwand added the 🚧 WIP Work-in-progress label Jul 5, 2023
@andiwand andiwand removed this from the next milestone Jul 5, 2023
@paulgessinger paulgessinger added the Breaking change This change breaks backwards compatibility label Jul 5, 2023
@paulgessinger paulgessinger added this to the next-major milestone Jul 5, 2023
@andiwand andiwand removed the 🚧 WIP Work-in-progress label Jul 5, 2023
@paulgessinger
Copy link
Member

We're ready to go for v28 now. Can you update the PR so we can merge it?

@paulgessinger
Copy link
Member

@benjaminhuth could you help review this?

benjaminhuth
benjaminhuth previously approved these changes Jul 24, 2023
@benjaminhuth
Copy link
Member

There are a bunch of conflicts @andiwand

@kodiakhq kodiakhq bot merged commit aa860d7 into acts-project:main Jul 27, 2023
53 checks passed
@andiwand andiwand deleted the rename-singletrackparam-to-generic branch July 27, 2023 15:48
@acts-project-service
Copy link
Collaborator

acts-project-service commented Jul 27, 2023

🔴 Athena integration test results

Build job with this PR failed!

Please investigate the build job for the pipeline!

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change This change breaks backwards compatibility Breaks Athena build This PR breaks the Athena build Component - Core Affects the Core module Component - Documentation Affects the documentation Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Component - Plugins Affects one or more Plugins Event Data Model Seeding SP formation Track Finding Track Fitting Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants