Repository navigation
Conversation
plexoos
force-pushed
the
rm-g4cx
branch
2 times, most recently
from
October 16, 2026 19:35
d2ef6d4 to
f4a2ebc
Compare
plexoos
marked this pull request as ready for review
October 16, 2026 23:10
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Missing event resets break repeated runs, while failed initialization retries and negative event IDs can terminate the process.
4 open issues
What changed in this PR
Introduces an installed, function-based Simphony API over geometry conversion and GPU execution, replacing direct G4CX usage in owned frontends.
Changes:
- Adds backend policies, lifecycle validation, and explicit geometry serialization.
- Migrates frontends to the public API.
- Adds API behavior testing and library installation rules.
| File | Description |
|---|---|
| tests/SimphonyApiTest.cc | Tests initialization and geometry serialization. |
| tests/CMakeLists.txt | Registers the API test. |
| src/GPURaytrace.h | Migrates initialization and simulation. |
| src/GPUPhotonSourceMinimal.h | Migrates simulation and reset calls. |
| src/GPUPhotonFileSource.h | Migrates simulation and reset calls. |
| src/GPUCerenkov.h | Migrates initialization and simulation. |
| src/g4app.h | Uses the facade for event execution. |
| src/consgeo.cc | Converts and saves geometry without a GPU backend. |
| src/CMakeLists.txt | Builds, exports, and installs the facade. |
| src/api/simphony.h | Declares the public runtime interface. |
| src/api/simphony.cc | Implements backend selection and lifecycle management. |
Review effort: Balanced
Introduce an installed function-based facade over U4 geometry conversion and CSGOptiX execution so applications can use Simphony without depending on the G4CX coordinator. - Export simphony::simphony and the documented simphony.h interface for initialization, backend queries, simulation/reset, and geometry saving. - Express GPU requirements through Required, Optional, and Disabled policies. Validate process-wide initialization, idempotent reuse, and pending-event/reset sequencing with descriptive exceptions. - Define the documented simphony::SensorIdentifier protocol in src/api, include it from simphony.h, and install it with the public headers. Update U4Tree, its default identifier, and the legacy G4CX/DD4hep sensor adapters to use the same interface without adding a U4-to-runtime link. - Migrate the Simphony-owned src frontends to the runtime while retaining G4CX and the existing external example/DD4hep execution paths for the follow-up retirement change. - Cover custom sensor dispatch, changed-identifier rejection, geometry-only initialization/export, lifecycle errors, and migrated executable ELF dependencies in the API and integration regressions. - Honor CMAKE_INSTALL_PREFIX when locating installed configuration data. BREAKING CHANGE: replace U4SensorIdentifier with simphony::SensorIdentifier from SensorIdentifier.h. Applications implementing a custom sensor mapping must update their base class and include; the legacy G4CX setter accepts the new interface while the coordinator remains available in this commit.
GPUCerenkov and GPURaytrace retain event 0 while reading its hits but never release it. A second /run/beamOn therefore fails the public runtime's pending-event check, including when opening the first run's output fails. Reset event 0 after consuming the hits and before returning from an output-open failure. Keep simulation reset disabled until hit retrieval has finished so cleanup does not discard results prematurely. Add executable regressions for two consecutive runs in both frontends, with normal output and with a directory blocking the hit-file path. All four cases reproduced the pending-event exception before this fix and now pass with nonzero GPU hits on both runs. Refs: #457
Geometry translation and backend creation populate process-global SSim, U4Tree, and CSG state before the public runtime is marked initialized. If an exception escapes that sequence, retrying initialization can append to the same global geometry and abort on a lower-level invariant. Record a failed lifecycle state when runtime construction throws, preserve the original exception, and reject subsequent initialization attempts with a descriptive logic_error requiring a process restart. Keep null-world validation and GPU-availability checks before this failure boundary so preflight failures do not prevent a later valid initialization. Document the failure contract and add a GPU-independent regression that throws from the sensor callback after real geometry translation, verifies that initialized/GPU state remains unpublished, and retries with matching and changed options. The test reproduced an assertion before the fix; both API initialization tests now pass. Refs: #457
SEvt asserts that event indices are non-negative. Forwarding simulate(-1) through an initialized GPU runtime can therefore abort the process instead of reporting invalid API input. Validate event IDs at the start of simulate and reset, before lifecycle checks or backend dispatch, and throw std::invalid_argument for negative values. Document the non-negative contract and cover rejection both before initialization and after geometry-only initialization, without requiring a GPU for invalid-input testing. The API regression failed with the old lifecycle error before this fix; both API tests now pass. Refs: #457
plexoos
added a commit
that referenced
this pull request
Oct 18, 2026
Follow-up to #457, now merged into main. The public runtime, sensor interface, and API regression tests were introduced there; this PR removes G4CX and migrates its remaining consumers. Remove the legacy G4CX coordinator and move its remaining consumers to `simphony::simphony`. This completes the migration of CSG test setup, DD4hep run/event actions, and the MC-truth and async examples to the same initialization and event lifecycle used by `simg4ox`. - Delete G4CX sources, installed target/headers, legacy executables/tests, top-level build entries, and logger hooks. - Preserve GDML name handling and auxiliary sensitive-detector tags in CSG tests while initializing the runtime with the GPU disabled. - Reuse the DD4hep runtime across runs, preserve hit injection and end-of-run batch flushing, and export geometry through `SaveGeometry` plus `GeometryDirectory`. - Migrate standalone examples and explicitly reset retained events after result retrieval, including synchronous and output-file failure paths. - Keep in-tree and standalone DD4hep builds working with C++20 by removing inherited raw standard flags from DD4hep's Geant4 helper target. - Update external-build, saved-geometry, and application-capability documentation and replace retired geometry-test export paths with `consgeo` exports. Validation: - Full DD4hep-enabled build and all 255 enabled CTest tests passed; two existing tests remain disabled. - Clean Release build/install with testing disabled contains no G4CX library, headers, exported target, or runtime dependency. Installed `consgeo` export passed. - All four standalone examples built against the clean package. MC-truth and both async examples completed repeated runs, including synchronous and batched modes. - DD4hep v01-38 / ROOT 6.36.14 / Geant4 11.4.2: in-tree and standalone plugin builds, standalone installation, and CPU/GPU raindrop tests passed. With the same GPU kernel, in-tree and standalone hit counts matched. Consecutive runs, geometry export, and end-of-run batch flushing passed. - The local Geant4 build uses initial-exec TLS, so these DD4hep checks used its documented TLS-check override and Geant4 preloading. Breaking change: consumers of `simphony::G4CX` / `G4CXOpticks` must link `simphony::simphony`, include `simphony.h`, initialize from a live Geant4 world, and reset retained events after reading results. Saved-foundry rendering and simtrace remain available through lower-level CSGOptiX tools. The three migrated standalone examples explicitly move from C++17 to C++20.
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.
Introduce an installed, function-based runtime over U4 geometry conversion and CSGOptiX execution. Applications initialize a live Geant4 world through
simphony::simphony, query GPU availability, simulate/reset events, and explicitly save geometry.simphony.handSensorIdentifier.hpublic headers. Applications provide custom sensor mappings throughsimphony::SensorIdentifierwithout including U4 implementation headers.Required,Optional, andDisabledGPU policies, idempotent initialization with matching inputs, and explicit pending-event/reset checks.src/applications to the runtime. The external-example and DD4hep run/event migrations are in the stacked G4CX retirement PR.CMAKE_INSTALL_PREFIXfor installed configuration data.Validation: independently built the API commit with the retained G4CX library,
simg4ox,consgeo, and API/legacy geometry tests. All 22 API, retained-G4CX geometry, andsimg4oxintegration checks passed. The complete stack passed 255 enabled CTest tests and DD4hep in-tree/standalone runtime checks.Migration: custom identifiers now implement
simphony::SensorIdentifierfromSensorIdentifier.h; the oldU4SensorIdentifiertype/header is removed. The retained G4CX setter accepts the new protocol.Follow-up: #459 removes G4CX and migrates the remaining external examples and DD4hep run/event actions. It is stacked on this PR and targets
rm-g4cx.