Skip to content

fix: local variables in FieldMapB and use MT npsim in workflows - #1183

Merged
wdconinc merged 3 commits into
mainfrom
npsim-j
Nov 9, 2026
Merged

wdconinc merged 3 commits into
mainfrom
npsim-j

Conversation

@wdconinc

@wdconinc wdconinc commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

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 FieldMapB that this change exposed.

Capybara diffs with -j N and the FieldMapB fix

With --numberOfThreads $(getconf _NPROCESSORS_ONLN) (= 4 on GitHub runners), the capybara report showed non-zero diffs against main. The reference run and the PR run used the same main commit (7023314) and the same eic_xl:nightly image (e693f695…), so the thread count was the only difference.

  • -j1 and no option behave the same. DDSim only switches to G4MTRunManager when numberOfThreads > 1, and the default is 1. The diffs come from single-threaded vs. 4-thread runs.
  • Per-event seeding (--random.enableEventSeed) works with multiple threads:
    • geantino gun: identical output except EventHeader.timeStamp, which is wall-clock time (std::time(nullptr)) and always differs.
    • DIS: generator-level particles identical in 99/100 events, matched by eventNumber. With multiple threads, events are written out of order.
  • Charged-particle propagation in the field differs (before the fix):
    • chargedgeantino (no physics, no random numbers during tracking): primaries identical; vertex hits differ by ~1e-6 mm (median), track endpoints by up to ~1 cm.
    • DIS: in every event the first difference is a primary's endpoint (as small as 5e-13 relative), which then grows into different showers.
  • Cause: FieldMapB stored the interpolation cell indices (ir, ix, iy, iz) and weights (dr, dx, dy, dz) as data members. fieldComponents() wrote them via GetIndices() 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 marks GetIndices() const. Single-threaded results are unchanged.

Results with the fix (4 threads vs. single-threaded main reference)

From CI run 42518768904 at 77e0a86 (same code as the current head; the later commit only changes comments). Events are matched by eventNumber; EventHeader.timeStamp is ignored.

Sample Events with differing MCParticles
geantino, chargedgeantino, e, pi 0/100
DIS 5x41, Q²>1 0/100
DIS 10x100, Q²>1 1/100 (event 3)
DIS 10x100, Q²>1000 0/100
DIS 18x275, Q²>1 2/100 (events 1, 2)
DIS 18x275, Q²>1000 1/100 (event 1)
  • The e gun output is bit-identical in every column, in event order.
  • geantino and chargedgeantino 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.
  • The 4 remaining events are all events 1–3, which are the first events processed by worker threads 2–4. That points to state initialized during each thread's first event (for example lazily built tables or caches) rather than another data race. Not yet identified.

Status:

  • with the fix, -j N output matches the single-threaded reference, except the first events on worker threads (4/700 events)
  • two -j N runs reproduce each other exactly (not yet checked)

What is the urgency of this PR?

  • High (please describe reason below)
  • Medium
  • Low

What kind of change does this PR introduce?

  • Bug fix (issue: FieldMapB not thread-safe under npsim -j N)
  • New feature (issue: npsim -j)
  • Optimization (issue #__)
  • Updated constants (issue #__)
  • Updated documentation
  • other: __

Please check if any of the following apply

  • This PR introduces breaking changes. Please describe changes users need to make below.
  • This PR changes default behavior. Please describe changes below.
  • AI was used in preparing this PR. Please describe usage below.

AI (Claude Code) was used to rebase this branch, investigate the capybara diffs (comparing CI artifacts and logs event by event), and write the FieldMapB fix and this description.

🤖 Generated with Claude Code

https://claude.ai/code/session_019qEz4tkBwMVXTuHoR9kTEe

Copilot AI lite review requested due to automatic review settings September 23, 2026 13:27
@github-actions github-actions Bot added the topic: infrastructure Regarding build system, CI, CD label Sep 23, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 Medium severity

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 the npsim gun simulation workflow step.
  • Add --numberOfThreads $(getconf _NPROCESSORS_ONLN) to the profiled DIS npsim workflow 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.

Comment thread .github/workflows/linux-eic-shell.yml Resolved
…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
@veprbl veprbl changed the title feat: use npsim -j/--numberOfThreads in workflows fix: local variables in FieldMapB and use MT npsim in workflows Nov 8, 2026

wdconinc commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Remaining hit-order differences: consistent with AIDASoft/DD4hep#1643

Summary. With the FieldMapB fix, the remaining TrackerEndcapHits differences in the geantino and chargedgeantino samples contain the same hits, stored in a different order. The order of hits from each sensitive detector is unchanged; only the order in which those per-detector blocks are concatenated changes. That order is fixed for a whole process but differs between processes, including two single-threaded runs of the same commit. Multi-threading does not cause it; a -j N run is simply another process. The code path below explains it.

Data

Per-event comparison of CI artifacts: single-threaded main (run 42255066992) vs. -j 4 (run 42518768904), events matched by eventNumber, hits labeled by the system field of cellID.

craterlake has 6 <detector>s sharing the TrackerEndcapHits readout (compact/tracking/silicon_disks.xml: systems 68–70 on the N side, 77–79 on the P side).

Job (separate process) Block order seen in every event of that file
geantino, single-threaded 69, 70, 68
geantino, -j 4 69, 68, 70
chargedgeantino, single-threaded 69, 68, 70
chargedgeantino, -j 4 68, 69, 70
DIS 10x100, single-threaded and -j 4 77, 78, 69, 68, 79, 70
  • Reordered events: geantino 17/100, chargedgeantino 21/100. In all of them, sorting the hits of each event gives identical contents, and the order of hits within each system is preserved.
  • Each file uses one block order for all of its events, including events processed by different worker threads.
  • The order is not sorted by system ID or detector name, and it differs between the single-threaded geantino and chargedgeantino jobs at the same commit.
  • Capybara compares histograms, so it does not see this; event-by-event comparisons do.

Mechanism (DD4hep @ 4644ad2)

  1. Geant4GeometryInfo.h:135: std::map<SensitiveDetector, std::set<const TGeoVolume*>> sensitives
  2. Handle.h:118-120: Handle::operator< compares m_element pointers, so the map is ordered by the heap addresses of the SensitiveDetector objects.
  3. Geant4DetectorSensitivesConstruction.cpp:91-96: sensitive detectors are created by iterating that map. This sets the G4SDManager registration order, and therefore the hit-collection IDs in G4HCofThisEvent.
  4. Geant4OutputAction.cpp:69-71: saveCollection() is called in hit-collection ID order.
  5. Geant4Output2EDM4hep.cpp:638 (and :673 for calorimeters): hits from all collections with the same name are appended to m_trackerHits[colName]. So when detectors share a readout, the output order is the sensitive-detector creation order, which follows pointer order.

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

  • In Geant4DetectorSensitivesConstruction::constructSensitives, create sensitive detectors in a deterministic order. Options:
    • iterate Detector::sensitiveDetectors(), which is keyed by name, and look each one up in p->sensitives;
    • copy the keys into a vector and sort them by sd.name();
    • give Geant4GeometryInfo::sensitives a comparator by name.
  • Optional: check other pointer-keyed containers that are iterated rather than only looked up (Geant4GeometryInfo, Geant4Converter), e.g. regions and limit sets, for similar order effects. The inner std::set<const TGeoVolume*> only drives setSensitiveDetector and should not affect output order.
  • Not recommended: re-sorting in the output plugins, which would only hide the issue in one output format.

Suggested test

  • Compact file with at least 3 <detector>s sharing one tracker readout and one calorimeter readout. Shoot a particle gun so each event crosses all of them.
  • Run ddsim single-threaded and with -j 2 (or -j 4) using --random.seed and --random.enableEventSeed. Match events by eventNumber, since multi-threaded output is written in completion order. Require identical hit order per collection.
  • Multi-threading is a convenient trigger: in this PR, the single-threaded vs. -j 4 comparison showed the reordering in roughly 1 in 5 events with one ePIC geometry. A second single-threaded run, or a run with ASLR enabled vs. setarch -R, also works but is less reliable as a regression test.
  • This also exercises the multi-threaded path in CI.

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

@wdconinc
wdconinc added this pull request to the merge queue Nov 8, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Nov 8, 2026
@veprbl
veprbl added this pull request to the merge queue Nov 8, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Nov 9, 2026
@wdconinc
wdconinc added this pull request to the merge queue Nov 9, 2026
Merged via the queue into main with commit 54b6a32 Nov 9, 2026
205 of 207 checks passed
@wdconinc
wdconinc deleted the npsim-j branch November 9, 2026 05:10

This branch was previously deployed

1 inactive deployment
github-pages — 637ab6b9 Deployed Nov 8, 2026 by veprbl via deploy-artifacts-page #6251
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: infrastructure Regarding build system, CI, CD

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants