acts icon indicating copy to clipboard operation
acts copied to clipboard

refactor!: Use track container in Core `CKF` and pass track proxies to delegates

Open andiwand opened this issue 1 year ago • 17 comments

Wires tracks through Core CKF to track branches and passes those tracks into the branch stopper delegate. This allows the user to count experiment specific track quantities on those branches and then cut on those (e.g. pixel holes).

blocked by

  • https://github.com/acts-project/acts/pull/3187

andiwand avatar Apr 30 '24 18:04 andiwand

@timadye this should allow us to count experiment specific stopping criteria inside the branch stopper without adding state to it but rather store this information on the branches

andiwand avatar May 10 '24 12:05 andiwand

no significant performance changes measured

andiwand avatar May 15 '24 22:05 andiwand

  1. This seems a big change adding the track container just to be able to access the track proxy from the branch stopper. Is this the only benefit, or is it useful in the other delegates or for another reason?

Right I also think this is a big change but there are other benefits from that:

  • The branches in the CKF are now modeled as tracks in the track container. Instead of having activeTips which was only valid after we created a track state we can now talk about braches which do not necessarily have to have track states.
  • Because of that we can also attach the extrapolated track parameters at the target surface to the track within the CKF which was previously not possible. I exercised that in https://github.com/acts-project/acts/pull/3195.
  1. I had previously hoped that we could use the track state to count experiment specific track quantities as they were visited. Is this not possible?

This is possible by adding custom columns to the track container and counting the stats within the branch stopper now. Although I didn't exercise that yet.

  1. Before introducing such a change, it would be good to check that it actually can do what it claims to. Would it be possible to have an example in the ActsExamples branch stopper? An actually functional example might be to cut on the number of 1D or 2D measurements.

I agree, I will try to make some reasonable showcase in the Examples. I am not sure if we will be able to merge that but I guess it would be already valuable for just pointing at some code.

andiwand avatar May 21 '24 07:05 andiwand

Codecov Report

Attention: Patch coverage is 29.33333% with 53 lines in your changes missing coverage. Please review.

Project coverage is 47.66%. Comparing base (cf9d872) to head (6096e56). Report is 21 commits behind head on main.

:exclamation: Current head 6096e56 differs from pull request most recent head ffdf2cc

Please upload reports for the commit ffdf2cc to get more accurate results.

Files Patch % Lines
...de/Acts/TrackFinding/CombinatorialKalmanFilter.hpp 29.16% 24 Missing and 27 partials :warning:
...e/include/Acts/EventData/VectorMultiTrajectory.hpp 0.00% 0 Missing and 1 partial :warning:
...re/include/Acts/EventData/VectorTrackContainer.hpp 50.00% 0 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3161      +/-   ##
==========================================
+ Coverage   47.24%   47.66%   +0.42%     
==========================================
  Files         508      509       +1     
  Lines       30041    29422     -619     
  Branches    14586    14131     -455     
==========================================
- Hits        14192    14024     -168     
+ Misses       5375     5282      -93     
+ Partials    10474    10116     -358     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 08 '24 17:06 codecov[bot]

I fear the coverage will not be easy to recover because the new ifs handle an edge case where we reject the first measurement and potentially more and finally maybe all of them. Do you think we can override in this case @paulgessinger ?

andiwand avatar Jun 26 '24 07:06 andiwand

@andiwand I was just playing around with the setting for now. No need to overcomplicate our process right now. I updated the setting now (to 25%) and it should be updated when the CI runs the next time.

paulgessinger avatar Jun 26 '24 07:06 paulgessinger

@timadye one thing I noticed is that we have a hole cut now in the track selector and in the CkfConfig. I guess it is okay for the Examples for now because aligning this looks complicated

andiwand avatar Jun 28 '24 11:06 andiwand

yeah, and the pix/str hole cuts aren't eta-dependent. Unfortunately the Acts::TrackSelector is a core class, so we can't add detector-dependent selections to it. I think that could be improved, but that is a whole other thing.

timadye avatar Jun 28 '24 13:06 timadye

This has some output changes, and the ExaTrkX job seems to fail to build. Will you look into this @andiwand?

paulgessinger avatar Jul 01 '24 09:07 paulgessinger

I will have a look. I think the output changes appeared with https://github.com/acts-project/acts/pull/3161/commits/255906dbd1fc3ed5f8af2d0b374eb1370bc300ac and exatrkx failed for a while but I didn't realize it. I guess I just have to absorb the braking change there

andiwand avatar Jul 01 '24 09:07 andiwand

@andiwand I'm just a bit confused why that would be. Does it make sense to you?

paulgessinger avatar Jul 01 '24 15:07 paulgessinger

The output changed again after I refactored the tip state counting to use the track interface. Looks already way healthier than before. I don't know exactly what causes the difference now but my hunch is that the branch stopper behaves a little bit differently than before.

Looking at the track classification image Which shows a reduction in duplicate tracks which also affects all the other plots to change.

andiwand avatar Jul 02 '24 07:07 andiwand

@andiwand physmon changes largely don't seem to be very large.

paulgessinger avatar Jul 02 '24 09:07 paulgessinger

I will update the reference

andiwand avatar Jul 02 '24 13:07 andiwand

:white_check_mark: Athena integration test results [6643b83069296d264cc8d70a1315275220037cbb]

:white_check_mark: All tests successful

status job report
:green_circle: report_pull_request
:green_circle: run_art_test: test_data18_13TeV_1000evt
:green_circle: run_art_test: test_ttbarPU40_reco
:green_circle: test_ActsDumpGeometryIdentifiers
:green_circle: test_ActsCheckObjectCountsWorkflow
:green_circle: test_ActsEFTrackFit
:green_circle: test_ActsPersistifySeeds
:green_circle: test_ActsBenchmarkWithSpot
:green_circle: test_ActsAnalogueClustering
:green_circle: test_ActsWorkflowHeavyIons
:green_circle: test_ActsWorkflowFastTracking
:green_circle: test_ActsWorkflowCached
:green_circle: test_ActsWorkflow
:green_circle: test_ActsValidateAmbiguityResolution
:green_circle: test_ActsValidateResolvedTracks
:green_circle: test_ActsValidateTracks
:green_circle: test_ActsValidateActsCoreSpacePoints
:green_circle: test_ActsValidateActsSpacePoints
:green_circle: test_ActsValidateSeeds
:green_circle: test_ActsValidateOrthogonalSeeds
:green_circle: test_ActsValidateClusters
:green_circle: test_ActsPersistifyEDM
:green_circle: test_ActsGSFRefitting
:green_circle: test_ActsKfRefitting
:green_circle: test_ActsExtrapolationAlgTest
:green_circle: test_ActsITkTest
:green_circle: run_workflow_tests_run4_mc
:green_circle: run_workflow_tests_run2_mc
:green_circle: run_workflow_tests_run2_data
:green_circle: run_workflow_tests_run3_mc
:green_circle: run_workflow_tests_run3_data
:green_circle: run_unit_tests

acts-project-service avatar Jul 03 '24 14:07 acts-project-service

I executed test_ActsWorkflow manually with main--ACTS and it seems to work

andiwand avatar Jul 09 '24 09:07 andiwand

So we think it's just the inconsistency?

paulgessinger avatar Jul 09 '24 19:07 paulgessinger

I think so - I triggered another run with the modified CI setup. Lets see if that goes through. Otherwise I would suggest to merge these PRs at the end of the day to get them into the nightly and then fix potential problems afterwards.

andiwand avatar Jul 10 '24 06:07 andiwand

Athena is happy now

andiwand avatar Jul 10 '24 16:07 andiwand