fix: make Acts geometry errors fatal at loading time - #2887
Open
wdconinc wants to merge 6 commits into
Open
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
wdconinc
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes Acts geometry conversion errors fail fast during service initialization, instead of deferring exceptions until the first time the geometry provider is accessed during event processing. This improves debuggability and ensures CI catches geometry problems earlier (notably for 2DStrip geometry with newer Acts).
Changes:
- Remove deferred exception capture in
AlgorithmsInit_serviceso Acts geometry initialization exceptions propagate immediately. - Simplify
algorithms::ActsSvcby removing the storedexception_ptrfailure path and making it a pure provider holder.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/services/algorithms_init/AlgorithmsInit_service.h | Stops swallowing Acts geometry init exceptions so failures become fatal during service initialization. |
| src/algorithms/interfaces/ActsSvc.h | Removes the deferred-failure mechanism (exception_ptr) from the Acts service interface. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ound (#2891) ## Fix MCParticlesHeadOnFrameNoBeamFX overflow for particle gun events **Stacked on:** #2887 (acts-event-data-seed-deprecated-declarations) ### Problem Single-electron gun events with secondary neutrons produce extreme momentum values (~10³⁰⁸ GeV) in `MCParticlesHeadOnFrameNoBeamFX`, causing CI failures in capybara histogram generation: - https://github.com/eic/EICrecon/actions/runs/32655851820/job/97236980632?pr=2887 ### Root Cause The fallback logic (lines 67-75) incorrectly treats **secondary hadrons** from particle gun events as beam particles: 1. Electron gun event produces secondary neutron (E=0.94 GeV) from showering 2. Fallback finds neutron → sets `hasBeamHadron=true`, leaves `hasBeamLepton=false` 3. Old check `if (!hasBeamHadron && !hasBeamLepton)` only returns if **BOTH** are false 4. Code proceeds with: - `e_beam = (0,0,0,0)` (uninitialized) - `h_beam = (0.024, 0, 0.94, 0.94)` (from secondary neutron) 5. Boost calculation: β = (-0.024/0.94, 0, -0.94/0.94) = (-0.026, 0, **-1.0**) 6. **β² = 1.0006 > 1** → unphysical (faster than light) → extreme Lorentz factors → 10³⁰⁸ GeV ### Solution **One-character fix:** Change `&&` to `||` on line 98. ```cpp // Old (buggy): if (!hasBeamHadron && !hasBeamLepton) { // Only returns if BOTH false return; } // New (correct): if (!hasBeamHadron || !hasBeamLepton) { // Returns if EITHER false return; } ``` This is consistent with the check at line 62 and prevents unphysical boost calculations. ### Effect - **Particle gun events** (with or without secondary hadrons): `MCParticlesHeadOnFrameNoBeamFX` will be **empty** (no valid collision frame) - **DIS events** (both beams present): Boost transformation applied correctly as before ### Testing Verified with `rec_e_1GeV_20GeV_craterlake.edm4eic.root`: - **Before fix:** Events 7, 11, 61, 95 have momentum ~10³⁰⁸ GeV or NaN - **Input MCParticles:** Normal momentum ~1.4 GeV - **Expected after fix:** No extreme values (empty output collection for gun events) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
### Briefly, what does this PR introduce? Please link to any relevant presentations or discussions. This pull request updates the workflow configuration in `.github/workflows/linux-eic-shell.yml` to improve control over job error handling in the `eicrecon-dis` job. The main changes introduce a `continue-on-error` setting to the job matrix, allowing certain job runs (2DStrip in particular) to proceed even if errors occur. Workflow configuration improvements: * Added a `continue-on-error` parameter to the `eicrecon-dis` job, making it configurable per matrix entry. * Updated the job matrix to include `continue-on-error: [false]` by default, and set `continue-on-error: true` for the configuration using `sanitizer: ASAN`, allowing that job to continue on error. [[1]](diffhunk://#diff-1979202099e4ebc6d02b6dbd6ade89a6d9895896cd610bb3829b3b84f5239b38R1149-R1157) [[2]](diffhunk://#diff-1979202099e4ebc6d02b6dbd6ade89a6d9895896cd610bb3829b3b84f5239b38R1191) ### What is the urgency of this PR? - [x] High (please describe reason below) - [ ] Medium - [ ] Low In Acts v46, the 2DStrip geometry is not compatible with the existing non-2DStrip material map anymore (I.e. it probably never was compatible but now it's a hard failure). ### What kind of change does this PR introduce? - [x] Bug fix (issue #__) - [ ] New feature (issue #__) - [ ] Optimization (issue #__) - [ ] Updated parameters, constants (issue #__) - [ ] Updated documentation - [ ] other: __ ### Please check if any of the following apply - [ ] This PR requires changes to geometry (epic PR: __) - [ ] This PR requires changes to EDM4eic (EDM PR: __) - [ ] 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. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Remove deferred exception handling in ActsSvc initialization to ensure geometry conversion errors cause immediate application failure during service initialization rather than being deferred until event processing. This makes debugging easier and ensures CI catches geometry issues early. Fixes failures with 2DStrip geometry when upgrading to Acts 46.8.1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
wdconinc
force-pushed
the
acts-geometry-fail-eagerly
branch
from
August 24, 2026 13:36
372f81f to
de71442
Compare
Contributor
Capybara summary for PR 2887
Last updated 2026-08-24T11:27-04:00 de71442 |
Base automatically changed from
acts-event-data-seed-deprecated-declarations
to
main
August 24, 2026 19:46
Member
|
I think this will make framework unusable for geometries without a constructible Acts geometries. See benchmark failures. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Briefly, what does this PR introduce? Please link to any relevant presentations or discussions.
This PR removes deferred exception handling in ActsSvc initialization to ensure geometry conversion errors cause immediate application failure during service initialization rather than being deferred until event processing.
This makes debugging easier and ensures CI catches geometry issues early.
Fixes uncaught failures with 2DStrip geometry when upgrading to Acts 46.8.1 (https://github.com/eic/EICrecon/actions/runs/32646904753/job/97215623061?pr=2885#step:7:310).
What is the urgency of this PR?
What kind of change does this PR introduce?
Please check if any of the following apply
GitHub Copilot CLI with Claude Sonnet 4.5.