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 IVF and AMVF in examples #2144

Merged
merged 8 commits into from
May 26, 2023

Conversation

andiwand
Copy link
Contributor

IVF and AMVF use different seeders and the code diverged quite a bit in the examples. In this PR I try to realign them

@andiwand andiwand added Component - Core Affects the Core module Component - Examples Affects the Examples module labels May 24, 2023
@andiwand andiwand added this to the next milestone May 24, 2023
@andiwand andiwand added the 🚧 WIP Work-in-progress label May 24, 2023
@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #2144 (0bbaa78) into main (eac824f) will increase coverage by 0.03%.
The diff coverage is 15.00%.

@@            Coverage Diff             @@
##             main    #2144      +/-   ##
==========================================
+ Coverage   49.46%   49.50%   +0.03%     
==========================================
  Files         436      436              
  Lines       25129    25128       -1     
  Branches    11606    11605       -1     
==========================================
+ Hits        12431    12440       +9     
+ Misses       4464     4431      -33     
- Partials     8234     8257      +23     
Impacted Files Coverage Δ
...e/include/Acts/Vertexing/IterativeVertexFinder.hpp 80.00% <ø> (ø)
Core/include/Acts/Vertexing/ZScanVertexFinder.hpp 90.90% <ø> (ø)
...e/include/Acts/Vertexing/IterativeVertexFinder.ipp 27.20% <15.00%> (+3.69%) ⬆️

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

@github-actions
Copy link

github-actions bot commented May 24, 2023

📊 Physics performance monitoring for 0bbaa78

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

Copy link
Contributor

@pbutti pbutti left a comment

Choose a reason for hiding this comment

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

As discussed on mattermost seems like the whole performance improve comes from a different seeder. The IVF algorithm with the track weights hardcoded at 1 seems like it's still not complete. But I think that could be addressed in a different PR. I would discourage to actually use the the cutOffTrackWeightReassign parameter if the trackWeights are not properly set. (probably disable it until then?)

@andiwand andiwand requested a review from pbutti May 24, 2023 17:38
@andiwand andiwand removed the 🚧 WIP Work-in-progress label May 25, 2023
@kodiakhq kodiakhq bot merged commit 210824a into acts-project:main May 26, 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.

I know it's merged already, but anyway

VertexSeeder seeder(seederCfg);
IPEstimator::Config ipEstCfg(m_cfg.bField, propagator);
IPEstimator ipEst(ipEstCfg);
Seeder seeder;
Copy link
Member

Choose a reason for hiding this comment

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

No config anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I aligned this with the AMVF. Now we use the TrackDensityVertexFinder which seems to go fine with the default constructed GaussianTrackDensity

@andiwand andiwand deleted the align-examples-ivf-amvf branch May 27, 2023 05:54
@paulgessinger paulgessinger modified the milestones: next, v27.0.0 Jun 20, 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 Component - Examples Affects the Examples module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants