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.

Sign up for GitHub

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

Jump to bottom

refactor!: Return all track states from Core CKF #3391

Merged
merged 25 commits into from
Sep 27, 2024
Merged

refactor!: Return all track states from Core CKF #3391

merged 25 commits into from
Sep 27, 2024

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Jul 16, 2024 •
edited
Loading

Since the Core CKF is growing into its own components which is then absorbed by a track finding algorithm it also allows us to extend tracks into different regions of the detector. While extending tracks we map want to keep eventual holes/material/outliers states at the end or beginning of the track. For this reason the CKF is modified to return all the states it found.

For convenience of the user I added helper functions which allow to trim tracks. This is then exercised in the Examples track finding.

blocked by

@andiwand andiwand added this to the next milestone Jul 16, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Track Finding labels Jul 16, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 16, 2024 •
edited
Loading

📊: Physics performance monitoring for baa41c7

Full contents

physmon summary

@andiwand
Copy link
Contributor Author

Discussed this briefly with @noemina - Potentially this should rather be a flag and this flag also toggles if we store anything before the first measurement. In this case we can also store holes at the end of the track.

cc @paulgessinger

@andiwand andiwand added the 🚧 WIP Work-in-progress label Jul 16, 2024
@paulgessinger paulgessinger modified the milestones: next, v36.0.0 Jul 18, 2024
@andiwand andiwand marked this pull request as draft July 23, 2024 08:56
@andiwand andiwand changed the title refactor: Keep outliers and material states at the end of track in Core CKF refactor: Return all track states from Core CKF Aug 2, 2024
@github-actions github-actions bot added the Component - Examples Affects the Examples module label Aug 2, 2024
@andiwand andiwand modified the milestones: next, v37.0.0 Aug 28, 2024
@andiwand andiwand changed the title refactor: Return all track states from Core CKF refactor!: Return all track states from Core CKF Aug 28, 2024
@andiwand andiwand mentioned this pull request Aug 29, 2024
Closed
This was referenced Sep 24, 2024
Merged
Merged
@andiwand andiwand added the 🛑 blocked This item is blocked by another item label Sep 25, 2024
@timadye
Copy link
Contributor

timadye commented Sep 25, 2024

I have checked #3644, #3648, and #3391 (this PR) and compared against main (65aff94). I rebased #3644 and merged #3391 with main to allow a fair comparison. With 100 ttbar_pu200 events, full_chain_itk.py gave the following results:

CPU [s/ev] speedup efficiency fake rate
main 1.72 – 99.19% 3.03%
#3648 1.69 1.4% 99.19% 3.02%
#3644 1.72 -0.4% 99.27% 3.43%
#3391 1.70 0.8% 99.27% 3.42%

where the CPU time is just for TrackFindingAlgorithm and is with 8 threads on the same machine. The speedup in each case is relative to main.

trackeff_vs_eta
fakerate_vs_eta
nHoles_vs_eta
nOutliers_vs_eta

I think this shows that the tracking performance changes come from #3644, while #3648 improves the speed slightly. #3391 includes the changes from the other two, but doesn't have any additional effect on CPU or performance.

@andiwand
Copy link
Contributor Author

Should we go ahead with #3391 and dump #3644 and #3648 @timadye ? Or do you prefer to merge them sequentially #3644 #3648 #3391?

@timadye
Copy link
Contributor

timadye commented Sep 25, 2024

I think it is cleaner to merge #3644 and #3648 (can be done in parallel), then #3391.

kodiakhq bot pushed a commit that referenced this pull request Sep 25, 2024
@andiwand
fix: Reject outliers while trimming track states in Core CKF (#3644)
0bf2465 
Currently CKF tracks can have outliers at the back because the measurement flag is also set for outliers as they contains such information. An additional check is introduced to reject outliers.

This is an attempt to also understand the output changes of #3391 better
paulgessinger pushed a commit that referenced this pull request Sep 26, 2024
@andiwand
fix: Reject material states when no measurements are found yet in Cor…
4a62f68 
…e CKF (#3648)

Currently CKF tracks can have material states before any measurement was
found. This is not true for outliers and holes. The logic is reworked so
material, outliers and holes are handled in the same way if we have not
found any measurements yet.

This is an attempt to also understand the output changes of
#3391 better
@andiwand andiwand removed the 🛑 blocked This item is blocked by another item label Sep 26, 2024
@andiwand andiwand requested a review from timadye September 26, 2024 09:12
@andiwand
Copy link
Contributor Author

@timadye the physmon diff disappeared now

@timadye
Copy link
Contributor

timadye commented Sep 26, 2024 •
edited
Loading

With 100 ttbar_pu200 events running full_chain_itk.py, #3391 TrackFindingAlgorithm is 1% faster than its base main (495c60d). I don't see any appreciable change in the efficiency or fakes.

@andiwand
Copy link
Contributor Author

With 100 ttbar_pu200 events running full_chain_itk.py, #3470 TrackFindingAlgorithm is 1% faster than its base main (495c60d). I don't see any appreciable change in the efficiency or fakes.

@timadye is this meant for #3470 or this PR ( #3391 ) ?

@timadye
Copy link
Contributor

timadye commented Sep 27, 2024

@timadye is this meant for #3470 or this PR ( #3391 ) ?

sorry for the typo. I have fixed the original comment. It was #3391.

@acts-project-service
Copy link
Collaborator

acts-project-service commented Sep 27, 2024 •
edited
Loading

✅ Athena integration test results [2196648]

✅ All tests successful

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

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Sep 27, 2024
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
48.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Sep 27, 2024
@kodiakhq kodiakhq bot merged commit 2196648 into acts-project:main Sep 27, 2024
46 checks passed
@andiwand andiwand deleted the refactor-ckf-keep-outliers-material-end-of-track branch September 27, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timadye timadye timadye approved these changes

Assignees
No one assigned
Labels
Breaks Athena build This PR breaks the Athena build Changes Performance Component - Core Affects the Core module Component - Examples Affects the Examples module Fails Athena tests This PR causes a failure in the Athena tests Track Finding
Projects
None yet
Milestone
v37.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@andiwand @timadye @acts-project-service @paulgessinger