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!: Wire time to spacepoints and seeds #2829

Merged
merged 13 commits into from
Jan 16, 2024

Conversation

andiwand
Copy link
Contributor

Currently our spacepoints and seeds do not carry any time information from a potential measurement to the initial track parameters.

In this PR I try to wire optional time information originating from the measurement, carried through spacepoints into seeds and finally into initial track params.

One caveat is that we use bound parameters in between which cannot tell if time is actually measured or not. My intermediate solution was to rely on the projected covariance entry to be 0 in this case.

Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (b418aee) 48.84% compared to head (bb43441) 48.86%.
Report is 1 commits behind head on main.

Files Patch % Lines
...s/SpacePointFormation/detail/SpacePointBuilder.ipp 53.84% 0 Missing and 6 partials ⚠️
Core/src/Utilities/SpacePointUtility.cpp 42.85% 0 Missing and 4 partials ⚠️
Core/include/Acts/Seeding/BinnedSPGroup.ipp 0.00% 2 Missing ⚠️
Core/src/Surfaces/ConeSurface.cpp 0.00% 0 Missing and 2 partials ⚠️
...clude/Acts/Seeding/EstimateTrackParamsFromSeed.hpp 66.66% 0 Missing and 1 partial ⚠️
Core/include/Acts/Seeding/InternalSpacePoint.hpp 0.00% 1 Missing ⚠️
Core/src/Surfaces/DiscSurface.cpp 50.00% 0 Missing and 1 partial ⚠️
Core/src/Surfaces/Surface.cpp 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2829      +/-   ##
==========================================
+ Coverage   48.84%   48.86%   +0.01%     
==========================================
  Files         491      491              
  Lines       28529    28532       +3     
  Branches    13477    13474       -3     
==========================================
+ Hits        13936    13942       +6     
- Misses       4868     4869       +1     
+ Partials     9725     9721       -4     

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

paulgessinger
paulgessinger previously approved these changes Dec 22, 2023
Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Cool. Is the statement in the description still the case? I are in principle the river is passed through as optional and the parameter estimation uses this information directly.

Core/include/Acts/Seeding/InternalSpacePoint.hpp Outdated Show resolved Hide resolved
@andiwand andiwand marked this pull request as ready for review December 22, 2023 09:30
@andiwand andiwand changed the title feat: Wire time to spacepoints and seeds feat!: Wire time to spacepoints and seeds Dec 22, 2023
@github-actions github-actions bot added the Component - Plugins Affects one or more Plugins label Dec 22, 2023
@paulgessinger
Copy link
Member

Physmon looks reasonable I think. Some other tests are still failing, otherwise I'd say this is good to go.

@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Dec 22, 2023
@kodiakhq kodiakhq bot merged commit 95d0052 into acts-project:main Jan 16, 2024
52 checks passed
@andiwand andiwand deleted the wire-time-to-sp-and-seeds branch January 16, 2024 18:21
@acts-project-service
Copy link
Collaborator

🔴 Athena integration test results [95d0052]

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 Jan 16, 2024
@paulgessinger paulgessinger modified the milestones: next, v32.0.0 Jan 19, 2024
@paulgessinger paulgessinger added Breaks Athena build This PR breaks the Athena build and removed Breaks Athena build This PR breaks the Athena build labels Jan 23, 2024
kodiakhq bot pushed a commit that referenced this pull request Jan 24, 2024
An initial track parameter covariance is necessary to avoid double counting and to estimate the covariance purely from the measurements.

Pulled out of #2086 to see the individual effects.

I added this to the ODD and ITk full chain and our physmon

bocked by
- #2829
LaraCalic pushed a commit to LaraCalic/acts that referenced this pull request Feb 10, 2024
Currently our spacepoints and seeds do not carry any time information from a potential measurement to the initial track parameters.

In this PR I try to wire optional time information originating from the measurement, carried through spacepoints into seeds and finally into initial track params.

One caveat is that we use bound parameters in between which cannot tell if time is actually measured or not. My intermediate solution was to rely on the projected covariance entry to be 0 in this case.
LaraCalic pushed a commit to LaraCalic/acts that referenced this pull request Feb 10, 2024
An initial track parameter covariance is necessary to avoid double counting and to estimate the covariance purely from the measurements.

Pulled out of acts-project#2086 to see the individual effects.

I added this to the ODD and ITk full chain and our physmon

bocked by
- acts-project#2829
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 Changes Performance Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Event Data Model Infrastructure Changes to build tools, continous integration, ... Seeding SP formation Track Finding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants