Skip to content
This repository was archived by the owner on Mar 9, 2026. It is now read-only.

Add possibility to run CKF Tracking on multiple threads#17

Merged
tmadlener merged 80 commits intoMuonColliderSoft:mainfrom
samf25:parallel
Mar 9, 2026
Merged

Add possibility to run CKF Tracking on multiple threads#17
tmadlener merged 80 commits intoMuonColliderSoft:mainfrom
samf25:parallel

Conversation

@samf25
Copy link
Copy Markdown

@samf25 samf25 commented Dec 2, 2025

I made a number of changes to the algorithms in k4ActsTracking.

  1. I improved the TruthTracking alg. Previously, it wasn't actually making use of its maps' runtime speed ups. I also used tbb's threading for the beefy for loop. This took the runtime (with BIB) from ~1000s to 0.5141s.
  2. I used stable sort and memory reserving instead of the insertion method in the DuplicateRemoval to decrease runtime (I think this is $N^2 \rightarrow N\log N$).
  3. I parallelized the CKFTrackingAlg's bottle neck loops (hit loop and seeding loop). This brings the runtime under control $\left(\frac{1}{n_{Threads}}\right)$ without increasing the memory demands (see plot).
WithBIBNaive

samf25 and others added 30 commits September 23, 2025 09:48
It's weird that this didn't pull with the rest of the changes ...
This only makes things build with v32 and v43, there are certainly
versions in between where different changes would be necessary.

(Potentially incomplete) list of related ACTS PRs:
- acts-project/acts#3337
- acts-project/acts#4005
Removed ROOT, EDM4hep, and Gaudi from the list of acts.
- Modified ACTSAlgBase to update geometry configuration
- Added buildsub/, installsub/, and setup.sh to gitignore
@samf25
Copy link
Copy Markdown
Author

samf25 commented Dec 3, 2025

The steering files for these new, parallel algs can be found here.
These steering files also use the thread-safe version of k4GaudiPandora

@madbaron
Copy link
Copy Markdown

Hi @samf25,

we've merged the PR with the previous set of updates into the key4hep/main.
When you have a moment, could you take a look at resolving the conflicts here?

We'll then merge this into our main, and start the PR to propagate the changes upstream.
Once we are done with this, I think we'll be able to close our fork, so any additional updates should target https://github.com/key4hep/k4ActsTracking directly.

@samf25
Copy link
Copy Markdown
Author

samf25 commented Jan 15, 2026

@madbaron Merge conflicts resolved. I believe I caught all the places where @tmadlener's changes to the SourceLink TrackerHit structure (Thank you for those by the way! It was bothering me the way I was doing it before) would affect my changes.

@samf25
Copy link
Copy Markdown
Author

samf25 commented Feb 23, 2026

There was a bug in the TrackTruthAlg where, if no prior entry existed in the map, it dereferenced the end of the map. Now it updates correctly.

Copy link
Copy Markdown

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Unless you have made any changes to any of the files in the data directory these can be removed again. Since key4hep#25 these are all downloaded via cmake in order to not pollute the git repo too much.

Otherwise this looks good to me. There are a few minor things that could be done. I have also added a few questions for clarification.

hit1.getEDep() == hit2.getEDep() && hit1.getEDepError() == hit2.getEDepError() &&
hit1.getPosition() == hit2.getPosition();
}
bool hitEqual(const edm4hep::TrackerHit& hit1, const edm4hep::TrackerHit& hit2) { return hit1 == hit2; }
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks like there is only one use of this in track_equal. Since the workaround is not necessary we could remove it entirely and simply use a std::find there, I think.

Comment thread k4ActsTracking/src/components/ACTSSeededCKFTrackingAlg.cxx
Comment thread k4ActsTracking/src/components/ACTSSeededCKFTrackingAlg.cxx Outdated
Comment thread k4ActsTracking/src/components/ACTSSeededCKFTrackingAlg.cxx
Comment thread k4ActsTracking/src/components/ACTSSeededCKFTrackingAlg.cxx
Comment thread k4ActsTracking/src/components/ACTSSeededCKFTrackingAlg.cxx Outdated
Comment thread k4ActsTracking/src/components/ACTSSeededCKFTrackingAlg.cxx Outdated
Copy link
Copy Markdown

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

I just had another look, a few minor things below. The main thing that is blocking me from merging at the moment is the presence of all the data files. Did you change any of those?

Comment thread k4ActsTracking/src/components/ACTSDuplicateRemoval.cxx Outdated
Comment thread k4ActsTracking/src/components/ACTSSeededCKFTrackingAlg.cxx
Comment on lines +531 to +533
for (size_t i = 0; i < spacePointGroups.size(); ++i) {
parallelSeedingAndTracking(tbb::blocked_range<size_t>(i, i + 1));
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Out of curiosity, does this impose a penalty timewise on the sequential execution compared to not using tbb? I suppose there would at least be some slowdown due to all the (needless) synchronization primitives that now also happen on a single thread, but I am not sure if this would be visible in our case since we do so much other stuff.

Comment thread k4ActsTracking/src/components/ACTSSeededCKFTrackingAlg.cxx
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
@samf25
Copy link
Copy Markdown
Author

samf25 commented Mar 2, 2026

As for the data dir, I haven't messed with it. I remember you saying that you wanted to take it out entirely and just move it to k4geo. Is this what you want to do?

@tmadlener
Copy link
Copy Markdown

The files are already automatically downloaded:

set(DATA_BASE_URL "https://key4hep.web.cern.ch/key4hep/k4ActsTracking/MuColl/data")
set(FILE_LIST_PATH "${CMAKE_CURRENT_SOURCE_DIR}/file_list.txt")
set(OUTPUT_DIR "${CMAKE_CURRENT_SOURCE_DIR}")
message(STATUS "Running download script for data files...")
execute_process(
COMMAND bash "${CMAKE_CURRENT_SOURCE_DIR}/download_files.sh" ${FILE_LIST_PATH} ${DATA_BASE_URL} ${OUTPUT_DIR}
RESULT_VARIABLE download_result
ERROR_VARIABLE download_error
)
if(download_result EQUAL 0)
message(STATUS "Data files download completed successfully")
else()
message(FATAL_ERROR "Failed to download data files:\n${download_error}")
endif()

this has been introduced in the upstream and then merged back here.

@samf25
Copy link
Copy Markdown
Author

samf25 commented Mar 2, 2026

Ah great, so I should just clear out all the files in the data dir? Or remove the whole dir itself?

@tmadlener
Copy link
Copy Markdown

Clear out the full data dir, but leave the CMakeLists.txt, file_list.txt and download_files.sh in there. Effectively all .json and .root files can go as they will be fetched during cmake.

@tmadlener tmadlener changed the title Parallel Optimization Add possibility to run CKF Tracking on multiple threads Mar 9, 2026
@tmadlener tmadlener merged commit bcf3a2c into MuonColliderSoft:main Mar 9, 2026
3 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants