From 3bfa4ebb6067762749c63a531fadce2e08be06ed Mon Sep 17 00:00:00 2001 From: salman Date: Fri, 14 Aug 2026 16:13:37 +0200 Subject: [PATCH] Add modernization todo suggestions. --- musrsim_modernization_todo.md | 1314 +++++++++++++++++++++++++++++++++ 1 file changed, 1314 insertions(+) create mode 100644 musrsim_modernization_todo.md diff --git a/musrsim_modernization_todo.md b/musrsim_modernization_todo.md new file mode 100644 index 0000000..fa2d227 --- /dev/null +++ b/musrsim_modernization_todo.md @@ -0,0 +1,1314 @@ +# musrSim Cleanup and Modernization TODO + +**Baseline:** `cleanup branch` +**Baseline date:** 2026-08-14 +**Goal:** Modernize and harden musrSim incrementally while preserving existing simulation physics and ROOT output compatibility unless a change is explicitly intended and regression-tested. + +--- + +## Guiding principles + +- Preserve physics behavior unless a change is intentional, reviewed, and regression-tested. +- Fix correctness and lifetime issues before broad stylistic refactoring. +- Prefer small, reviewable patches over large rewrites. +- Add regression coverage before refactoring sensitive code. +- Keep ROOT branch names and file structure stable unless a migration is explicitly planned. +- Avoid introducing Geant4 multithreading until singleton/global state and ROOT output handling are substantially cleaned up. +- Avoid wholesale replacement of the current physics-list architecture until stronger differential tests are in place. + +--- + +# Phase 1 - Correctness fixes + +These items should be addressed before larger refactoring. + +## 1.1 Initialize `zangleSigma` + +**Priority:** Critical +**Area:** `musrPrimaryGeneratorAction` + +### Problem + +`zangleSigma` is used to decide whether the cosine angular distribution is selected: + +```cpp +if (zangleSigma < 0) { + ... +} +``` + +but it is not initialized in the constructor. + +This can make generator behavior depend on indeterminate memory when `/gun/tiltsigma` is not specified. + +### Required modification + +Initialize `zangleSigma` explicitly in the constructor initializer list, for example: + +```cpp +xangleSigma(0), +yangleSigma(0), +zangleSigma(0), +pitch(0), +``` + +### Acceptance criteria + +- [ ] `zangleSigma` is initialized explicitly. +- [ ] Default generator behavior is deterministic. +- [ ] Add a regression test for default angular behavior with no `/gun/tiltsigma`. + +--- + +## 1.2 Repair `F04GlobalField` lifetime and ownership + +**Priority:** Critical +**Area:** `F04GlobalField`, `musrRunAction`, `F04ElementField` + +### Problems + +`musrRunAction::EndOfRunAction()` deletes the singleton instance: + +```cpp +if (F04GlobalField::Exists()) { + F04GlobalField* myGlobalField = F04GlobalField::getObject(); + if (myGlobalField != nullptr) { + delete myGlobalField; + } +} +``` + +but the singleton pointer is not reset by the destructor. + +This can leave a dangling singleton pointer and is particularly dangerous when multiple `/run/beamOn` commands occur in one process. + +Additionally, element fields are deleted through `F04ElementField*`, but the base class should have a virtual destructor. + +### Required modifications + +- Add a virtual destructor to `F04ElementField`: + +```cpp +virtual ~F04ElementField() = default; +``` + +- Ensure destruction of `F04GlobalField` resets the static singleton pointer. +- Reconsider whether `F04GlobalField` should be deleted at `EndOfRunAction()` at all. +- Prefer an application/geometry lifetime over a per-run lifetime if consistent with Geant4 ownership. +- Verify all owned members have clear destruction semantics. + +### Acceptance criteria + +- [ ] No dangling singleton pointer after destruction. +- [ ] `F04ElementField` has a virtual destructor. +- [ ] Two consecutive `/run/beamOn` commands work safely. +- [ ] Field objects are destroyed exactly once. +- [ ] Add a regression test with multiple runs in one process. + +--- + +## 1.3 Separate field-update logic from field deletion + +**Priority:** High +**Area:** `F04GlobalField` + +### Problem + +`updateField()` currently calls `clear()`, and `clear()` deletes registered element fields. + +Updating integration parameters such as the stepper or chord finder should not implicitly delete the physical field definitions. + +### Required modification + +Separate responsibilities, for example: + +```text +rebuildIntegrationInfrastructure() +clearElementFields() +``` + +Possible structure: + +- `clearElementFields()`: + - deletes or clears registered element fields only. +- `rebuildIntegrationInfrastructure()`: + - recreates equation/stepper/chord finder as needed. + - does not alter registered field geometry. +- `updateField()`: + - calls only the infrastructure rebuild required by changed settings. + +### Acceptance criteria + +- [ ] `/field/update` does not delete configured element fields. +- [ ] Repeated field updates do not leak equations, steppers, or chord finders. +- [ ] Add a regression test that updates field settings after fields are configured. + +--- + +## 1.4 Fix `trd90y` with `norot` + +**Priority:** High +**Area:** `musrDetectorConstruction` + +### Problem + +For `trd90y`, code applies: + +```cpp +pRot->rotateY(90.0 * CLHEP::deg); +``` + +but `pRot` can be `nullptr` when the macro specifies `norot`. + +### Required modification + +Use a local or owned identity rotation matrix when `trd90y` requires an intrinsic rotation and no external rotation matrix was supplied. + +Example concept: + +```cpp +G4RotationMatrix localRotation; +G4RotationMatrix* effectiveRotation = pRot; + +if (command == "trd90y") { + localRotation.rotateY(90.0 * CLHEP::deg); + if (pRot != nullptr) { + localRotation = (*pRot) * localRotation; + } + effectiveRotation = &localRotation; +} +``` + +The exact implementation should respect Geant4 placement lifetime requirements. + +### Acceptance criteria + +- [ ] `construct trd90y ... norot ...` does not crash. +- [ ] Existing explicit-rotation behavior is preserved. +- [ ] Add a minimal geometry regression macro. + +--- + +## 1.5 Prevent optical-photon ROOT output overflow + +**Priority:** Critical +**Area:** `musrRootOutput` + +### Problem + +`phot_time` has a fixed maximum size: + +```cpp +Double_t phot_time[maxNOptPhotDet]; +``` + +but `nOptPhotDet` continues increasing even after storage capacity is exceeded. + +ROOT then uses `nOptPhotDet` as the array length: + +```cpp +"phot_time[nOptPhotDet]/D" +``` + +which can cause an out-of-bounds read during `Fill()`. + +### Required modification + +Separate total detected photons from stored photon times, for example: + +```cpp +int nOptPhotDetTotal; +int nOptPhotDetStored; +``` + +Then: + +- increment `nOptPhotDetTotal` for every detected photon; +- append/store time only while `nOptPhotDetStored < maxNOptPhotDet`; +- use `nOptPhotDetStored` as the ROOT leaf-array length; +- optionally write both counters to ROOT. + +Alternative: migrate this branch to a `std::vector` if ROOT compatibility requirements allow it. + +### Acceptance criteria + +- [ ] ROOT never reads beyond `phot_time`. +- [ ] Total detected-photon count remains available. +- [ ] Stored-photon count is unambiguous. +- [ ] Behavior beyond the storage limit is documented. +- [ ] Add a regression test with more than `maxNOptPhotDet` detections. +- [ ] Run under AddressSanitizer if available. + +--- + +## 1.6 Fix APD index bounds + +**Priority:** High +**Area:** `musrScintSD::FindAPDcellID` + +### Problem + +Checks such as: + +```cpp +ix > APDcell_nx +``` + +allow `ix == APDcell_nx`, which is outside the valid range. + +The correct range is: + +```text +0 ... APDcell_nx - 1 +``` + +### Required modifications + +Use: + +```cpp +ix < 0 || ix >= APDcell_nx +``` + +and equivalent checks for y and z. + +Also validate: + +```cpp +APDcell_nx > 0 +APDcell_ny > 0 +APDcell_nz > 0 +``` + +before computing cell dimensions. + +### Acceptance criteria + +- [ ] Exact upper-bound coordinates are handled correctly. +- [ ] Invalid zero/negative APD dimensions are rejected. +- [ ] Boundary tests exist for all axes. + +--- + +## 1.7 Protect GPS mode from incompatible `/gun/*` commands + +**Priority:** Critical +**Area:** `musrPrimaryGeneratorAction`, `musrPrimaryGeneratorMessenger` + +### Problem + +In GPS mode, `particleSource` is created instead of `particleGun`. + +However, the `/gun/` messenger remains installed and some handlers dereference `particleGun`, for example: + +```cpp +particleGun->GetParticleDefinition() +particleGun->SetParticleDefinition(...) +``` + +This can crash when GPS and incompatible `/gun/*` commands are mixed. + +### Required modification + +Choose one clear interface policy: + +### Preferred option + +Do not expose ParticleGun-specific commands while GPS mode is active. + +### Minimum safe option + +Guard all ParticleGun-only methods: + +```cpp +if (!particleGun) { + // report a clear configuration error + return; +} +``` + +Also classify generator commands into: + +- common generator controls; +- ParticleGun-only controls; +- GPS-only controls. + +### Acceptance criteria + +- [ ] No `/gun/*` command can dereference a null `particleGun`. +- [ ] Incompatible commands produce a clear error or warning. +- [ ] Documentation matches actual behavior. +- [ ] Add a GPS + incompatible `/gun/*` regression test. + +--- + +## 1.8 Replace manual filename manipulation in `musrParameters` + +**Priority:** Medium +**Area:** `musrParameters` + +### Problem + +Filename generation uses string-position arithmetic such as: + +```cpp +myStopFileName.replace( + myStopFileName.length()-3, + myStopFileName.length()-1, + "stop"); +``` + +This assumes filename length and extension layout and misuses the second `replace()` parameter as though it were an end position. + +### Required modification + +Use C++17 `std::filesystem::path`: + +```cpp +std::filesystem::path steering{steeringFileName}; + +auto stopFile = steering; +stopFile.replace_extension(".stop"); + +auto randomFile = steering; +randomFile.replace_extension(".rndm"); +``` + +### Acceptance criteria + +- [ ] Works with short filenames. +- [ ] Works with filenames containing multiple dots. +- [ ] Works with nested directory paths. +- [ ] Existing expected filenames remain unchanged where intended. +- [ ] Add unit tests for representative paths. + +--- + +## 1.9 Make electric-field units explicit + +**Priority:** High +**Area:** `musrDetectorConstruction`, `musrTabulatedElementField` + +### Problem + +The field parser applies: + +```cpp +fieldValue * CLHEP::tesla +``` + +even when creating an electric field. + +The code itself contains a comment indicating this may be incorrect. + +### Required modification + +Introduce an explicit field type, for example: + +```cpp +enum class FieldKind { + Magnetic, + Electric +}; +``` + +Choose units according to field kind: + +```cpp +Magnetic -> CLHEP::tesla +Electric -> CLHEP::kilovolt / CLHEP::mm +``` + +Apply the same distinction consistently to: + +- nominal field values; +- field ramps; +- ROOT output; +- documentation; +- interpolation/map loading if applicable. + +### Acceptance criteria + +- [ ] Magnetic and electric field units are represented explicitly. +- [ ] No electric quantity is expressed using a magnetic-field unit symbol in code. +- [ ] Add known-value field-map tests. +- [ ] Verify existing magnetic-field output remains unchanged. + +--- + +# Phase 2 - Parser and core-architecture hardening + +## 2.1 Validate every steering-file extraction + +**Priority:** High +**Area:** steering/configuration parsing + +### Problem + +The code has improved substantially by removing much of the old `sscanf` parsing, but many operations still look like: + +```cpp +double pp1, pp2, pp3; +lineStream >> matrixName >> pp1 >> pp2 >> pp3; +``` + +without checking stream state. + +Malformed input can therefore leave values invalid or indeterminate. + +### Required modification + +Use checked extraction and report precise diagnostics. + +Recommended source representation: + +```cpp +struct SourceLine { + std::size_t number; + std::string text; +}; +``` + +Recommended error format: + +```text +geometry.mac:137: construct tubs: +expected 17 arguments, received 14 +``` + +### Acceptance criteria + +- [ ] Every parser branch validates required parameters. +- [ ] Errors include filename, line number, and command. +- [ ] Missing/invalid numeric values do not reach Geant4 constructors. +- [ ] Unit tests cover malformed macros. + +--- + +## 2.2 Break up `ParseConstructCommand()` + +**Priority:** Medium-High +**Area:** `musrDetectorConstruction` + +### Problem + +The geometry parser remains very large and couples parsing, validation, lookup, and object construction. + +### Required modification + +Split shape-specific parsing into focused functions, for example: + +```cpp +ParseTube(...) +ParseBox(...) +ParseTrd(...) +ParsePolycone(...) +ParseTorus(...) +``` + +Prefer a common parsed-definition layer before Geant4 construction. + +Possible longer-term representation: + +```cpp +struct TubeDefinition { + std::string name; + double rMin; + double rMax; + double halfLength; + ... +}; +``` + +### Acceptance criteria + +- [ ] Main construct parser becomes dispatch-oriented. +- [ ] Shape-specific validation is localized. +- [ ] Existing macros remain compatible. +- [ ] Regression geometry outputs remain unchanged. + +--- + +## 2.3 Stop using `std::map::operator[]` for read-only lookups + +**Priority:** Medium-High +**Area:** mappings and event lookups + +### Problem + +Patterns such as: + +```cpp +saveVolumeMapping[actualVolume] +SensDetectorMapping[logivol] +globalChangeFieldInStepsMap[eventNumber] +``` + +insert missing keys. + +This can: + +- hide configuration errors; +- mutate state during queries; +- grow maps during simulation; +- make zero indistinguishable from "not found". + +### Required modification + +Use: + +```cpp +auto it = map.find(key); +``` + +or: + +```cpp +map.at(key) +``` + +where absence is an error. + +Pay particular attention to: + +- sensitive-detector mapping; +- saved-volume mapping; +- volume weighting; +- field lookups; +- event-number field-change lookup; +- rotation-matrix lookup. + +### Acceptance criteria + +- [ ] Read-only queries no longer mutate maps. +- [ ] Missing configuration is handled explicitly. +- [ ] Per-event maps do not grow simply because events are queried. +- [ ] Tests cover missing-key behavior. + +--- + +## 2.4 Investigate `pointerToField` + +**Priority:** Medium +**Area:** geometry/local-field handling + +### Problem + +`pointerToField` appears to be queried but may not be populated in the current implementation. + +### Required investigation + +Determine whether: + +1. this is dead support for an old local-field mechanism; +2. population occurs indirectly and is simply hard to find; +3. current local-field support is incomplete. + +### Actions + +- [ ] Trace every write and read of `pointerToField`. +- [ ] Add a local-field integration test if the feature is active. +- [ ] Remove the map and related syntax if the feature is obsolete. +- [ ] Otherwise restore/clarify initialization and document it. + +--- + +## 2.5 Establish a consistent fatal-error policy + +**Priority:** Medium +**Area:** whole core + +### Problem + +There are still many direct `exit()` calls. + +Direct process termination makes testing difficult and obscures cleanup/ownership semantics. + +### Required modification + +Adopt a consistent policy: + +```text +steering/configuration error + -> configuration exception + +Geant4 runtime fatal condition + -> G4Exception + +main() + -> catches configuration exceptions and returns non-zero +``` + +Avoid mechanically replacing every `exit()` without understanding semantics. + +### Acceptance criteria + +- [ ] Parser failures are testable without terminating the test process. +- [ ] Fatal Geant4 errors use the Geant4 error mechanism where appropriate. +- [ ] Resource cleanup still occurs on failure. + +--- + +# Phase 3 - ROOT output and state cleanup + +## 3.1 Modernize `musrRootOutput` internals while preserving ROOT compatibility + +**Priority:** Medium +**Area:** `musrRootOutput` + +### Problem + +The class contains many synchronized parallel arrays: + +```cpp +det_ID[] +det_edep[] +det_time_start[] +det_time_end[] +... +``` + +and repetitive setters. + +This increases the risk of index synchronization errors. + +### Required modification + +Internally group related values into structures, for example: + +```cpp +struct DetectorHit { + int id; + double edep; + double timeStart; + double timeEnd; + ... +}; +``` + +Possible strategies: + +- retain fixed ROOT buffers but populate them from structured internal data; +- use `std::array` for fixed buffers; +- use `std::vector` only where ROOT schema compatibility permits. + +### Important constraint + +Do not change existing ROOT branch names/types without an explicit migration decision. + +### Acceptance criteria + +- [ ] Existing ROOT files remain schema-compatible. +- [ ] Differential ROOT-output tests show no unintended changes. +- [ ] Fixed-capacity overflows are handled explicitly. +- [ ] Repetitive setter logic is reduced. + +--- + +## 3.2 Simplify CFD/optical output structures + +**Priority:** Medium +**Area:** optical detector output + +### Problem + +There are many separate members/setters such as CFD threshold arrays and special timing fields. + +### Required modification + +Use indexed internal arrays or containers where values are logically homogeneous. + +Example: + +```cpp +std::array cfdTimes; +``` + +Map these onto existing ROOT branch names during tree setup. + +### Acceptance criteria + +- [ ] ROOT schema remains unchanged. +- [ ] Repetitive code is substantially reduced. +- [ ] Threshold/index mappings are tested. + +--- + +## 3.3 Reduce global singleton/static state + +**Priority:** Medium +**Area:** application architecture + +### Current examples + +- `F04GlobalField` +- `musrRootOutput` +- `musrParameters` +- `musrErrorMessage` +- `musrScintSD` +- `musrSteppingAction` +- `musrEventAction` +- dynamically allocated global/static seed vector + +### Required approach + +Do this incrementally. + +Start with simple state such as random-seed storage, then move toward explicit constructor dependencies where practical. + +Do not combine singleton removal with a Geant4 multithreading migration. + +### Acceptance criteria + +- [ ] Random-seed storage no longer requires a global heap pointer. +- [ ] Ownership of shared services is documented. +- [ ] New code avoids adding additional singleton dependencies. + +--- + +# Phase 4 - Build, testing, and developer tooling + +## 4.1 Expand regression tests + +**Priority:** High + +Add focused tests for: + +- [ ] default `zangleSigma`; +- [ ] two consecutive `/run/beamOn` runs; +- [ ] `trd90y` with `norot`; +- [ ] GPS + incompatible `/gun/*`; +- [ ] more than `maxNOptPhotDet` optical detections; +- [ ] APD boundary coordinates; +- [ ] malformed geometry parameters; +- [ ] electric-field map units; +- [ ] unknown mapping lookup without insertion; +- [ ] TURTLE starting-line semantics; +- [ ] local-field behavior if `pointerToField` remains supported. + +Where possible, use fixed seeds and compare only stable, meaningful quantities. + +--- + +## 4.2 Convert differential scripts into automated tests + +**Priority:** Medium-High + +Current differential/reference scripts should become reproducible test targets where practical. + +### Required modification + +- register stable comparisons with CTest; +- provide test data in the repository; +- clearly separate: + - exact comparisons; + - floating-point tolerant comparisons; + - statistical/physics comparisons. + +### Acceptance criteria + +- [ ] A developer can run the standard suite with one command. +- [ ] CI can execute the same suite. +- [ ] Failures indicate which observable changed. + +--- + +## 4.3 Introduce a `musrSimCore` library + +**Priority:** Medium +**Area:** CMake + +### Motivation + +Tests currently need to compile or link selected implementation files independently. + +### Proposed structure + +```cmake +add_library(musrSimCore + ... +) + +add_executable(musrSim + musrSim.cc +) + +target_link_libraries(musrSim + PRIVATE musrSimCore +) +``` + +Tests then link against `musrSimCore`. + +### Acceptance criteria + +- [ ] Core implementation is compiled once. +- [ ] Unit/integration tests can link the core library. +- [ ] Public/private include boundaries are clearer. + +--- + +## 4.4 Add compiler warnings and sanitizer build options + +**Priority:** High for development builds + +Recommended warning set: + +```text +-Wall +-Wextra +-Wpedantic +``` + +Consider optional stricter warnings after the initial cleanup. + +Add developer CMake options for: + +- AddressSanitizer; +- UndefinedBehaviorSanitizer. + +Potentially ThreadSanitizer only much later if multithreading is introduced. + +### Acceptance criteria + +- [ ] Warning-enabled build is reasonably clean. +- [ ] ASan regression suite passes. +- [ ] UBSan regression suite passes. + +--- + +## 4.5 Modernize ROOT CMake integration + +**Priority:** Low-Medium + +Investigate replacing legacy/custom ROOT discovery with imported ROOT targets where supported: + +```cmake +find_package(ROOT CONFIG REQUIRED ...) +target_link_libraries(... ROOT::Core ROOT::Tree ...) +``` + +Keep compatibility requirements for PSI/build environments in mind. + +### Acceptance criteria + +- [ ] Supported ROOT versions configure successfully. +- [ ] No global ROOT link directories are required. +- [ ] CI/build documentation reflects the supported configuration. + +--- + +## 4.6 Generate version information from the build + +**Priority:** Low-Medium + +### Problem + +Version information appears in several places and can drift: + +- README; +- manual; +- executable banner; +- possibly CMake/package metadata. + +### Required modification + +Define the version once in CMake and generate a header: + +```cpp +#define MUSRSIM_VERSION ... +``` + +Optionally include: + +- Git commit hash; +- dirty-tree indicator; +- Geant4 version; +- ROOT version. + +### Acceptance criteria + +- [ ] One canonical musrSim version definition. +- [ ] Executable banner uses generated metadata. +- [ ] Documentation no longer hard-codes contradictory implementation versions. + +--- + +## 4.7 Add CI + +**Priority:** Medium + +Once builds are reproducible in the chosen environment: + +- configure; +- build; +- run unit tests; +- run selected integration tests; +- run warning build; +- optionally run ASan/UBSan jobs; +- verify documentation builds if dependencies are available. + +Keep runtime reasonable so CI remains useful. + +--- + +# Phase 5 - `musrSimAna` modernization + +Treat `musrSimAna` as a separate cleanup project after the main simulation code is stabilized. + +## 5.1 Fix CLI argument handling + +**Priority:** Critical + +### Current problems + +The program may assume `argv[2]` or `argv[3]` exists when it does not. + +The same positional argument can also be interpreted both as: + +- event count; +- `nographic`. + +### Required modification + +Define an unambiguous CLI. + +Possible direction: + +```text +musrSimAna [--events N] [--nographic] [--pileup] +``` + +A lightweight manual parser is acceptable if dependencies should remain minimal. + +### Acceptance criteria + +- [ ] No out-of-range `argv` access. +- [ ] Help output exactly matches accepted syntax. +- [ ] Invalid combinations produce useful errors. +- [ ] CLI parser has unit tests. + +--- + +## 5.2 Replace legacy string/file handling + +**Priority:** High + +Replace remaining uses of: + +- `sscanf`; +- `sprintf`; +- `strcpy`; +- `NULL`; +- C-style filename manipulation. + +Use: + +- `std::string`; +- `std::filesystem`; +- `std::getline`; +- `std::istringstream`; +- `nullptr`. + +### Acceptance criteria + +- [ ] No unsafe unbounded string copies. +- [ ] Config parser validates extraction. +- [ ] File paths use `std::filesystem` where appropriate. + +--- + +## 5.3 Fix ROOT file error checking + +**Priority:** High + +### Problem + +Checking: + +```cpp +if (f == NULL) +``` + +after: + +```cpp +new TFile(...) +``` + +does not correctly detect normal ROOT file-open failures. + +### Required modification + +Use ROOT's file-state API, for example checking for a zombie/error state. + +### Acceptance criteria + +- [ ] Missing/corrupt input files produce a clean error. +- [ ] No analysis proceeds with an invalid `TFile`. + +--- + +## 5.4 Introduce RAII in `musrSimAna` + +**Priority:** Medium + +Apply RAII to locally owned: + +- ROOT files; +- analysis helper objects; +- application/UI objects where ownership is local; +- configuration/state objects. + +Respect ROOT ownership conventions rather than blindly converting every pointer. + +--- + +## 5.5 Add `musrSimAna` to top-level CMake + +**Priority:** Medium + +### Goals + +- one build entry point; +- one dependency configuration; +- shared compiler settings; +- shared test infrastructure; +- optional build switch if analysis dependencies differ. + +Example: + +```cmake +option(BUILD_MUSRSIMANA "Build musrSimAna" ON) +``` + +--- + +## 5.6 Add analysis regression tests + +**Priority:** Medium + +Add tests for: + +- CLI parsing; +- setup-file parsing; +- missing input file; +- small known ROOT input; +- fixed expected histogram/integral values; +- `nographic` execution; +- event-count limit. + +--- + +# Phase 6 - Documentation synchronization + +Perform a full code-to-document pass after the interfaces above stabilize. + +## 6.1 Correct known documentation drift + +Items to verify/update include: + +- [ ] `M0`, `M1`, `M2` naming; +- [ ] supported geometry solids; +- [ ] generator command behavior; +- [ ] GPS vs `/gun/*`; +- [ ] TURTLE starting-line semantics; +- [ ] field units; +- [ ] ROOT branch/configuration-key mapping; +- [ ] optical output; +- [ ] event filters; +- [ ] current physics commands; +- [ ] `musrSimAna` CLI; +- [ ] current supported Geant4/ROOT versions; +- [ ] current example macro inventory. + +--- + +## 6.2 Remove or clearly label historical instructions + +Review documentation referring to: + +- Geant4 4.x/9.x migration; +- hard-coded old PSI/home paths; +- obsolete Geant4 processes; +- missing example macros; +- missing analysis figures. + +Move historically useful material into an explicitly historical section/file instead of mixing it with current instructions. + +--- + +## 6.3 Make examples executable documentation + +Create a small maintained examples set, for example: + +```text +examples/basic_geometry.mac +examples/gps.mac +examples/field.mac +examples/optical.mac +``` + +Run these in CI with small event counts. + +### Acceptance criteria + +- [ ] Every documented example exists. +- [ ] Example macros parse successfully. +- [ ] Selected examples run in CI. +- [ ] Documentation cannot silently drift away from available examples. + +--- + +## 6.4 Consider generating command-reference material from code + +**Priority:** Long-term improvement + +To reduce future drift, consider describing commands in a centralized registry containing: + +- command name; +- arguments; +- units; +- default values; +- help text; +- handler. + +Use the same metadata for: + +- parsing; +- runtime help; +- manual command-reference generation. + +A similar table can be used for ROOT output: + +```text +configuration key -> ROOT branch -> type -> unit -> default enabled state +``` + +--- + +# Phase 7 - Deferred larger modernization + +These should not be started until the earlier phases have good regression coverage. + +## 7.1 Physics-list architecture + +**Defer for now.** + +Do not replace the current physics implementation wholesale with `G4VModularPhysicsList` until: + +- physics outputs are well covered by differential tests; +- expected tolerances are defined; +- reference samples exist. + +Any such migration should be treated as a physics-validation project, not just a C++ cleanup. + +--- + +## 7.2 Geant4 multithreading + +**Defer for now.** + +Current global/singleton state and ROOT output assumptions make this a separate architectural project. + +Prerequisites should include: + +- reduced global state; +- clear per-run/per-event ownership; +- thread-safe ROOT output strategy; +- deterministic seed handling; +- stronger regression tests. + +--- + +# Suggested implementation order + +## Batch 1 - Immediate correctness + +- [ ] Initialize `zangleSigma` +- [ ] Fix optical-photon array/count overflow +- [ ] Fix `trd90y` + `norot` +- [ ] Fix APD bounds +- [ ] Guard GPS against incompatible `/gun/*` +- [ ] Add focused tests for each fix + +## Batch 2 - Field system + +- [ ] Add virtual destructor to `F04ElementField` +- [ ] Fix `F04GlobalField` singleton lifetime +- [ ] Separate `updateField()` from element-field deletion +- [ ] Fix electric-field unit handling +- [ ] Remove query-time map insertion in field-event lookup +- [ ] Add multiple-run and field-update regression tests + +## Batch 3 - Parser hardening + +- [ ] Introduce source filename/line tracking +- [ ] Validate every stream extraction +- [ ] Improve parser diagnostics +- [ ] Break up `ParseConstructCommand()` +- [ ] Replace manual filename manipulation with `std::filesystem` +- [ ] Add malformed-input tests + +## Batch 4 - State, mappings, and ROOT output + +- [ ] Replace read-only `map[]` usage +- [ ] Investigate/remove `pointerToField` +- [ ] Establish consistent exception/error policy +- [ ] Clean up `musrRootOutput` internals +- [ ] Simplify CFD/optical data structures +- [ ] Begin reducing singleton/static state +- [ ] Preserve ROOT schema with differential tests + +## Batch 5 - Build and test infrastructure + +- [ ] Introduce `musrSimCore` +- [ ] Add warning build +- [ ] Add ASan +- [ ] Add UBSan +- [ ] Automate differential tests +- [ ] Modernize ROOT CMake integration +- [ ] Generate version metadata from CMake +- [ ] Add CI + +## Batch 6 - `musrSimAna` + +- [ ] Fix CLI +- [ ] Modernize parser/string handling +- [ ] Fix ROOT input error handling +- [ ] Introduce RAII +- [ ] Integrate into top-level CMake +- [ ] Add analysis regression tests + +## Batch 7 - Documentation + +- [ ] Full code-to-manual synchronization +- [ ] Clean historical notes +- [ ] Maintain executable examples +- [ ] Consider generated command/ROOT reference tables + +## Batch 8 - Future architecture + +- [ ] Evaluate modular physics-list migration +- [ ] Evaluate Geant4 multithreading + +--- + +# Definition of "cleanup complete" + +The cleanup/modernization effort can reasonably be considered mature when: + +- [ ] No known undefined behavior or lifetime bugs remain in the sequential simulation path. +- [ ] Core builds cleanly with the agreed warning set. +- [ ] ASan and UBSan runs are clean for the regression suite. +- [ ] Steering-file errors include useful source locations and do not produce undefined behavior. +- [ ] Multiple `/run/beamOn` runs are safe. +- [ ] GPS and ParticleGun interfaces cannot be mixed unsafely. +- [ ] Fixed-capacity ROOT buffers cannot overflow or cause out-of-bounds ROOT reads. +- [ ] ROOT output compatibility is regression-tested. +- [ ] Major map lookups do not silently insert missing entries. +- [ ] Core tests and representative integration tests run automatically. +- [ ] `musrSimAna` has safe CLI/config parsing and is part of the normal build/test flow. +- [ ] User documentation matches the current implementation and available examples. +- [ ] Physics-list and multithreading changes remain explicitly separated from ordinary cleanup unless independently validated. +