refactor(genstep): retire legacy suffixed type codes - #429
Conversation
There was a problem hiding this comment.
Pull request overview
This PR completes the retirement of legacy, suffixed genstep type codes (historical layout/version tags) by removing their enum/string/classification handling and consolidating scintillation production/dispatch onto the single modern OpticksGenstep_SCINTILLATION path. It also updates U4’s collection API and call sites accordingly, and adds test coverage to lock in the intended behavior and stable protocol values.
Changes:
- Remove legacy genstep identifiers (previously occupying values 1–5) and associated name/type/classification branches, while keeping numeric protocol stability by leaving 1–5 unused.
- Rename U4 scintillation collection entry points and environment controls to revision-independent
CollectGenstep_Scintillation*, and switch producers to emitOpticksGenstep_SCINTILLATION. - Add a focused
OpticksGenstepTestto verify retired-code rejection, stable current IDs, classification, and photon-flag mapping.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| u4/U4.hh | Renames the public U4 scintillation genstep collection API and env-var keys to revision-independent names. |
| u4/U4.cc | Implements the renamed collection path and switches produced scintillation gensteps to OpticksGenstep_SCINTILLATION. |
| u4/Local_DsG4Scintillation.cc | Updates local scintillation stepping to call the renamed U4 collection API. |
| sysrap/tests/sgs_test.cc | Updates tests to use a non-retired genstep type constant. |
| sysrap/tests/OpticksGenstepTest.cc | Adds new unit test coverage for retired codes and stable current genstep identifiers. |
| sysrap/tests/CMakeLists.txt | Registers the new OpticksGenstepTest in the sysrap test build/run loop. |
| sysrap/sscint.h | Updates header commentary to reflect shared modern scintillation usage (collected + fabricated). |
| sysrap/OpticksGenstep.h | Removes legacy enum names/strings and legacy name/type/classification branches while preserving protocol values. |
| src/GPURaytrace.h | Updates stepping action to call U4::CollectGenstep_Scintillation. |
| src/GPUCerenkov.h | Updates stepping action to call U4::CollectGenstep_Scintillation. |
| qudarap/qsim.h | Removes the obsolete r4695 dispatch alias and relies on the generic scintillation dispatch case. |
| examples/MC_Truth/GPUMCTruth.h | Updates example stepping code to call the renamed U4 collection API. |
| examples/async_gpu_std/async_gpu_std.h | Updates example genstep emission to OpticksGenstep_SCINTILLATION and call sites to renamed API. |
| examples/async_gpu_launch/async_gpu_launch.h | Updates example genstep emission to OpticksGenstep_SCINTILLATION and call sites to renamed API. |
| dd4hepplugins/OpticsSteppingAction.cc | Mechanically updates the DD4hep stepping action call site to the renamed U4 API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6389f77 to
cc736b5
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
cc736b5 to
a980153
Compare
Motivation: The five low-valued Geant4 and DsG4 genstep identifiers encode the upstream version or revision that originally introduced their layouts, rather than distinct behavior still supported by simphony. Investigation showed that codes 1-4 survived only in name parsing and classification helpers. The remaining r4695 scintillation code had live producers and GPU dispatch, but used the same sscint representation and generator as the generic SCINTILLATION type. Keeping these aliases made current data describe historical implementations, obscured which protocol variants are genuinely supported, and required a revision-qualified U4 API for ordinary scintillation collection. Since compatibility with old saved genstep.npy arrays is not required, retire those encodings instead of preserving dead aliases. Values 1–4 are still recognized by `Name`, `IsCerenkov`, `IsScintillation`, and `GenstepToPhotonFlag` (`sysrap/OpticksGenstep.h:128`). That gives them limited diagnostic/host-side classification behavior. However: - There are no current producers for them. - OpticksGenstep_::Type() has no callers. - Input genstep files are accepted based only on shape (sysrap/SEvt.cc:402). - On the GPU, none of values 1–4 has a dispatch case. They fall into generate_photon_dummy, which emits placeholder data (qudarap/qsim.h:155, qudarap/qsim.h:2434). So their apparent legacy-file compatibility is incomplete and potentially misleading. The suffixes are historical format/version tags: - 1042 means the layout derived from Geant4 10.4.p02. - r3971 and r4695 appear to identify revisions of detector-specific DsG4* implementations. Current code explicitly calls r4695 the JUNO-specific variant (sysrap/sscint.h:9). I found no authoritative in-tree statement identifying exactly which upstream revision-control repository those numbers came from. - They were introduced in commit 2bae5bc (2bae5bc) as “genstep versioning for clarity.” The old OptiX generators were removed in 2024 and the legacy producers in 2025. Historical context: - 2bae5bc (2020-06-21, "start adding genstep versioning for clarity") introduced codes 1-4 as version-qualified G4 10.4.2 and DsG4 r3971 layouts. - 062bc6d (2020-06-21) separated genstep codes from photon flags and partially ported the G4 10.4.2 scintillation representation. - c578cd4 (2021-06-21) added DsG4Scintillation_r4695 as code 5 and its G4Opticks collector, shifting TORCH and the later protocol values upward. - 3b467a1 (2021-06-23) described the old-style r4695 generation path itself as a "quickfix" and a "stopgap prior to new qudarap approach being ready." - 5902876 (2022-05-05) introduced the revision-independent CERENKOV and SCINTILLATION codes 15 and 16 for the modern generator layouts. - 0d21dfd (2022-05-13) introduced lower-level sscint/U4 collection: fabricated sscint gensteps already used generic SCINTILLATION, while the U4 collector retained the historical r4695 label. - 62435a1 (2022-08-10) routed r4695 and generic SCINTILLATION through the same qsim scintillation generator, confirming that the old code had become a compatibility alias rather than a separate algorithm. Thus codes 1-4 are remnants of retired layouts, and code 5 is an obsolete alias for the generic scintillation path. The current change completes the migration begun by the 2022 generic genstep work. Implementation: - remove the five suffixed enum names and their string, parsing, naming, and classification branches - leave numeric values 1-5 intentionally unused so current genstep protocol values are not renumbered - emit OpticksGenstep_SCINTILLATION from U4 and standalone producers and use the single generic qsim dispatch path - rename U4::CollectGenstep_DsG4Scintillation_r4695 and its environment controls to revision-independent scintillation names - update all source, example, and DD4hep call sites - add OpticksGenstepTest coverage for rejected retired codes, stable current IDs, classification, and photon-flag mapping This intentionally makes gensteps carrying codes 1-5 invalid and removes the old U4 API and environment-variable names. DD4hep was not available in the local build, so its mechanically updated call site was not compiled.
a980153 to
fee3134
Compare
Align OpticksGenstep_UsesMaterialLine with the current genstep protocol after #429 retired the five legacy suffixed identifiers. Only the generic Cerenkov, scintillation, and modified G4 Cerenkov producers retain the q0.z material-line contract. Cover numeric protocol slots 1 through 5 as retired non-material-line gensteps so a future reintroduction cannot silently restore the stale classification. Validated by rebuilding CSGOptiX7_ptx and OpticksGenstepTest and running SysRapTest.OpticksGenstepTest plus SysRapTest.GenstepTest. Refs: #346 Refs: #429
Motivation:
The five low-valued Geant4 and DsG4 genstep identifiers encode the upstream version or revision that originally introduced their layouts, rather than distinct behavior still supported by simphony. Investigation showed that codes 1-4 survived only in name parsing and classification helpers. The remaining r4695 scintillation code had live producers and GPU dispatch, but used the same sscint representation and generator as the generic SCINTILLATION type.
Keeping these aliases made current data describe historical implementations, obscured which protocol variants are genuinely supported, and required a revision-qualified U4 API for ordinary scintillation collection. Since compatibility with old saved genstep.npy arrays is not required, retire those encodings instead of preserving dead aliases.
Values 1–4 are still recognized by
Name,IsCerenkov,IsScintillation, andGenstepToPhotonFlag(sysrap/OpticksGenstep.h:128). That gives them limited diagnostic/host-side classification behavior. However:So their apparent legacy-file compatibility is incomplete and potentially misleading.
The suffixes are historical format/version tags:
1042 means the layout derived from Geant4 10.4.p02.
r3971 and r4695 appear to identify revisions of detector-specific DsG4* implementations. Current code explicitly calls r4695 the JUNO-specific variant (sysrap/sscint.h:9). I found no authoritative in-tree statement identifying exactly which upstream revision-control repository those
numbers came from.
They were introduced in commit 2bae5bc (2bae5bc) as “genstep versioning for clarity.” The old OptiX generators were removed in 2024 and the legacy producers in 2025.
Historical context:
Thus codes 1-4 are remnants of retired layouts, and code 5 is an obsolete alias for the generic scintillation path. The current change completes the migration begun by the 2022 generic genstep work.
Implementation:
This intentionally makes gensteps carrying codes 1-5 invalid and removes the old U4 API and environment-variable names.
DD4hep was not available in the local build, so its mechanically updated call site was not compiled.