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: change in ITk seed confirmation for high pT muons efficiency #1956

Merged
merged 9 commits into from
Mar 28, 2023

Conversation

LuisFelipeCoelho
Copy link
Member

As discussed by @noemina in https://indico.cern.ch/event/1258598/contributions/5286608/attachments/2607062/4503967/UpgradeTracking_230308.pdf
and Athena MR https://gitlab.cern.ch:8443/atlas/athena/-/merge_requests/61367 this PR slightly changes the seed confirmation and ITk configuration (z binning and maximum distance between middle and bottom space points) to improve reconstruction for high pT muons

@LuisFelipeCoelho LuisFelipeCoelho added Component - Core Affects the Core module Improvement Changes to an existing feature 🛑 blocked This item is blocked by another item labels Mar 17, 2023
@LuisFelipeCoelho LuisFelipeCoelho added this to the next milestone Mar 17, 2023
Core/include/Acts/Seeding/SeedFilter.ipp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/SeedFinder.ipp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/SeedFinderOrthogonal.ipp Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Mar 17, 2023

📊 Physics performance monitoring for 3c414fa

Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: 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

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

@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #1956 (8ff6554) into main (4d7e280) will not change coverage.
The diff coverage is n/a.

❗ Current head 8ff6554 differs from pull request most recent head 3c414fa. Consider uploading reports for the commit 3c414fa to get more accurate results

@@           Coverage Diff           @@
##             main    #1956   +/-   ##
=======================================
  Coverage   49.80%   49.80%           
=======================================
  Files         413      413           
  Lines       23596    23596           
  Branches    10676    10676           
=======================================
  Hits        11753    11753           
  Misses       4342     4342           
  Partials     7501     7501           

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

@LuisFelipeCoelho
Copy link
Member Author

The change in ITk performance is expected so I replaced the root hash

@LuisFelipeCoelho LuisFelipeCoelho added 🛑 blocked This item is blocked by another item and removed 🛑 blocked This item is blocked by another item labels Mar 18, 2023
@LuisFelipeCoelho
Copy link
Member Author

I guess I can close this PR since it was reverted in Athena @noemina

@LuisFelipeCoelho
Copy link
Member Author

LuisFelipeCoelho commented Mar 21, 2023

Or actually I will keep only the changes from Carlo's comment and the change in config

@LuisFelipeCoelho LuisFelipeCoelho added the 🚧 WIP Work-in-progress label Mar 21, 2023
@CarloVarni
Copy link
Collaborator

@LuisFelipeCoelho news on this?

I see there are changes to the configuration, does that improve timing performance (how much?) while keeping high physics performance?

@LuisFelipeCoelho
Copy link
Member Author

These config changes were also changed in Athena:
https://gitlab.cern.ch:8443/atlas/athena/-/merge_requests/61367
and then part of that MR was reverted in
https://gitlab.cern.ch:8443/atlas/athena/-/merge_requests/61666#160ad76ded943bf4a5cbf52a2e53d999a1c0ec3a
because of bad time performance.

The config changes don't seem to affect the performance too much

@LuisFelipeCoelho LuisFelipeCoelho removed 🚧 WIP Work-in-progress 🛑 blocked This item is blocked by another item labels Mar 28, 2023
@CarloVarni
Copy link
Collaborator

just to confirm then... in Athena these changes produced worse performance? But in ACTS it is ok?

@LuisFelipeCoelho
Copy link
Member Author

LuisFelipeCoelho commented Mar 28, 2023

no, sorry. The seed confirmation changes in the first PR resulted in worse performance. These changes were reverted in Athena, but they kept the configuration changes, which does not affect performance too much.
Here in acts I am only applying the changes that were kept in Athena.

@kodiakhq kodiakhq bot merged commit e79cf07 into acts-project:main Mar 28, 2023
1 check passed
@paulgessinger paulgessinger modified the milestones: next, v25.0.0 Apr 21, 2023
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 Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants