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: Machine learning based Ambiguity Solver #1877

Merged
merged 40 commits into from
Mar 3, 2023

Conversation

Corentin-Allaire
Copy link
Contributor

This MR depends on #1868.

Implement a machine learning based ambiguity solver. If Acts is compiled with the ACTS_PLUGIN_ONNX option, it can be used in the odd full chain by simply adding the --MLSolver argument.

In addition to the implementation of the solver, this PR adds an option for the regular solver to only work on track with a certain amount of hits (to obtain fairer comparison).

This also add some small change to the Csv multitrajectory writing, it now also write the list of measurements associated with each track.

Finally, the python script to train the network, test the network and compute its performances are made available with the onnx file of the model.

I haven't written the documentation for this solver, yet it will be part of a future MR after this gets merged in.

@Corentin-Allaire Corentin-Allaire added this to the next milestone Feb 17, 2023
@Corentin-Allaire Corentin-Allaire added Impact - Minor Nuissance bug and/or affects only a single module Component - Examples Affects the Examples module Feature Development to integrate a new feature 🚧 WIP Work-in-progress labels Feb 17, 2023
@asalzburger
Copy link
Contributor

This is on top of the batch input for ONNX #1868 ?

@Corentin-Allaire
Copy link
Contributor Author

This is on top of the batch input for ONNX #1868 ?

Yes, the batch input caused a factor 7 speed up so I thought it should go in first. But I guess it is close to be merged

@asalzburger
Copy link
Contributor

Alright, I have seen that you had addressed @benjaminhuth s comments, so I merged #1868

@Corentin-Allaire Corentin-Allaire removed the 🚧 WIP Work-in-progress label Feb 17, 2023
@github-actions
Copy link

github-actions bot commented Feb 17, 2023

📊 Physics performance monitoring for 05f12d8

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 Feb 17, 2023

Codecov Report

Merging #1877 (05f12d8) into main (f35d49b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1877   +/-   ##
=======================================
  Coverage   49.48%   49.48%           
=======================================
  Files         408      408           
  Lines       22704    22704           
  Branches    10363    10363           
=======================================
  Hits        11236    11236           
  Misses       4257     4257           
  Partials     7211     7211           

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

@Corentin-Allaire
Copy link
Contributor Author

The jobs that don't pass the CI are related to the fact I have change the default solver to only consider track with at least 7 hits. I can put the threshold to 0 but honestly I would be concern by the quality of a track with < 7 hits.
If anybody has an opinion on this...

@asalzburger
Copy link
Contributor

e CI are rel

In that case, the PR should include the updated reference, what do you think? @paulgessinger

@paulgessinger
Copy link
Member

Is it obvious why the non-ambi outputs also change @Corentin-Allaire?

@Corentin-Allaire
Copy link
Contributor Author

Is it obvious why the non-ambi outputs also change @Corentin-Allaire?

I the odd-full-chain the vertexing uses the output of the Solver as an input. If you remove some tracks (the one with < 7 measurements) then it will affect the vertexing. What was a bit surprising is that it appear to improve the vertexing performance.

Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

in general the python scripts could share some common code

left a few other comments. nothing critical - as always feel free to ignore

@Corentin-Allaire
Copy link
Contributor Author

All of the comment should have been addressed now ! I made one tiny change to the file name for the Csv multitrajectory writing, it use to always be CKFtracks.csv now the naming depend on the algorithm that called the writer (so it is easier to write the output for the CKF and the solver at the same time) .

Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

great! thank you!

@kodiakhq kodiakhq bot merged commit a9278e3 into acts-project:main Mar 3, 2023
@github-actions github-actions bot removed the automerge label Mar 3, 2023
@paulgessinger paulgessinger modified the milestones: next, v23.5.0 Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Examples Affects the Examples module Feature Development to integrate a new feature Impact - Minor Nuissance bug and/or affects only a single module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants