Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The npsim-gun job’s matrix can run ddsim, but the new --numberOfThreads flag is applied unconditionally and may not be supported by ddsim, risking CI failures.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR updates the CI workflow to exercise npsim multithreading by passing a thread-count argument during simulation runs, improving throughput and ensuring threaded execution is covered in artifact diffs.
Changes:
- Add
--numberOfThreads $(getconf _NPROCESSORS_ONLN)to thenpsimgun simulation workflow step. - Add
--numberOfThreads $(getconf _NPROCESSORS_ONLN)to the profiled DISnpsimworkflow step.
| File | Description |
|---|---|
.github/workflows/linux-eic-shell.yml |
Adds npsim thread-count configuration to workflow simulation invocations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Capybara summary for PR 1183
Material map validation
|
…afety The cell indices (ir, ix, iy, iz) and interpolation weights (dr, dx, dy, dz) were data members of FieldMapB, written by GetIndices() and read back in fieldComponents(). The field object is shared between Geant4 worker threads, so with npsim -j N > 1 concurrent field evaluations could interpolate with another thread's cell and weights. Make them locals and mark GetIndices() const. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019qEz4tkBwMVXTuHoR9kTEe
npsim -j/--numberOfThreads in workflowsFieldMapB and use MT npsim in workflows
Remaining hit-order differences: consistent with AIDASoft/DD4hep#1643Summary. With the DataPer-event comparison of CI artifacts: single-threaded
Mechanism (DD4hep @ 4644ad2)
Hit order inside each Geant4 hit collection follows track traversal and is deterministic. Allocation addresses are not reproducible between processes (ASLR, separate allocator arenas for threads started by ROOT, XRootD and Geant4). Within a process the map is shared, so all worker threads agree on one order, as observed. Possible fix in DD4hep
Suggested test
Not yet verified: the reordering disappears after such a fix. The analysis is based on CI artifacts and the code path above; I have not run an instrumented build. Generated by Claude Code |

Briefly, what does this PR introduce? Please link to any relevant presentations or discussions.
This PR adds -j to the npsim runs in the workflows. Slightly more efficient but more importantly it's functionality we want to ensure is exercised in a place where we can see diffs.
It also fixes a thread-safety bug in
FieldMapBthat this change exposed.Capybara diffs with
-j Nand theFieldMapBfixWith
--numberOfThreads $(getconf _NPROCESSORS_ONLN)(= 4 on GitHub runners), the capybara report showed non-zero diffs againstmain. The reference run and the PR run used the samemaincommit (7023314) and the sameeic_xl:nightlyimage (e693f695…), so the thread count was the only difference.-j1and no option behave the same. DDSim only switches toG4MTRunManagerwhennumberOfThreads > 1, and the default is 1. The diffs come from single-threaded vs. 4-thread runs.--random.enableEventSeed) works with multiple threads:EventHeader.timeStamp, which is wall-clock time (std::time(nullptr)) and always differs.eventNumber. With multiple threads, events are written out of order.FieldMapBstored the interpolation cell indices (ir, ix, iy, iz) and weights (dr, dx, dy, dz) as data members.fieldComponents()wrote them viaGetIndices()and then read them back. DD4hep shares the field object across Geant4 worker threads, so concurrent evaluations could interpolate with another thread's cell and weights. This PR makes them locals and marksGetIndices()const. Single-threaded results are unchanged.Results with the fix (4 threads vs. single-threaded
mainreference)From CI run 42518768904 at
77e0a86(same code as the current head; the later commit only changes comments). Events are matched byeventNumber;EventHeader.timeStampis ignored.TrackerEndcapHits: the same hits, but stored in a different order within some events (17 and 21 events). Capybara compares histograms, so it is not affected.Status:
-j Noutput matches the single-threaded reference, except the first events on worker threads (4/700 events)-j Nruns reproduce each other exactly (not yet checked)What is the urgency of this PR?
What kind of change does this PR introduce?
FieldMapBnot thread-safe undernpsim -j N)Please check if any of the following apply
AI (Claude Code) was used to rebase this branch, investigate the capybara diffs (comparing CI artifacts and logs event by event), and write the
FieldMapBfix and this description.🤖 Generated with Claude Code
https://claude.ai/code/session_019qEz4tkBwMVXTuHoR9kTEe