refactor(config)!: make JSON authoritative for event settings by plexoos · Pull Request #401 · BNLNPPS/simphony

plexoos · 2026-07-06T21:15:36Z

Summary

Make typed simphony::Config defaults plus the selected JSON profile the authoritative source for modeled event settings.

Add typed JSON fields for retained event, resource, propagation, output, and input controls.
Apply the complete configuration deterministically through the existing SEventConfig facade before event creation.
Remove obsolete and legacy OPTICKS_* event-setting support, including values that were previously read and then silently overwritten.
Migrate shipped profiles, scripts, asynchronous examples, and DD4hep integration to named JSON configurations.
Document the complete event schema, defaults, discovery behavior, and configuration lifecycle.
Expand configuration tests and make source fixtures work when Simphony is embedded with add_subdirectory.

Breaking changes

Legacy OPTICKS_* variables for modeled event settings are no longer honored. Set these values in the selected JSON profile.
event.mode has been replaced by event.event_mode; the old key is rejected with an explanatory error.
Applications must construct simphony::Config before creating event or geometry consumers.

SIMPHONY_CONFIG_DIR and SIMPHONY_PTX_DIR remain supported as discovery variables; they are not event settings.

Migration

Select a configuration basename with -c or --config:

SIMPHONY_CONFIG_DIR=$PWD/config \
    GPUPhotonSourceMinimal \
    -g tests/geom/wls_test.gdml \
    -c wls_test \
    -m tests/run.mac

Move runtime settings into the profile’s event object:

{
  "event": {
    "event_mode": "HitPhoton",
    "integration_mode": 1,
    "max_bounce": 100,
    "maxslot": 100000,
    "output_dir": "./output"
  }
}

Scope

The legacy G4CX layer is not being modernized here because it is scheduled for removal in a separate PR.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6ff6222679

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

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".

Copilot

Pull request overview

This PR refactors the event persistence mode configuration key by renaming the JSON field from event.mode to event.event_mode, updating both the C++ config reader and the checked-in sample config files so that Config continues to drive SEventConfig correctly.

Changes:

Update Config::ReadConfig to read the event mode from event.event_mode instead of event.mode.
Reorder Config’s event_mode member placement and adjust Config::Apply() call ordering for SEventConfig::SetEventMode.
Update sample JSON configs to use "event_mode".

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
sysrap/config.h	Moves `event_mode` member (no semantic change by itself).
sysrap/config.cpp	Reads renamed JSON key and applies `SEventConfig::SetEventMode`.
config/wls_test.json	Updates event mode key to `"event_mode"`.
config/trap_iso.json	Updates event mode key to `"event_mode"`.
config/sphere_leak.json	Updates event mode key to `"event_mode"`.
config/dev.json	Updates event mode key to `"event_mode"` and reorders within the object.
config/8x8SiPM_crystal.json	Updates event mode key to `"event_mode"`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

🟡 Changes recommended

Standalone diagnostics still lack JSON initialization, and configuration fixture discovery breaks in embedded builds.

3 open issues

1 resolved since last review

Renaming the JSON key from event.mode to event.event_mode is a breaking schema change: older…

_{Review effort: Balanced}

+int         SEventConfig::_IntegrationMode = _IntegrationModeDefault;
+const char* SEventConfig::_EventMode = _EventModeDefault;
 const char* SEventConfig::_DeviceName  = nullptr ;
-int         SEventConfig::_RunningMode = SRM::Type(ssys::getenvvar(kRunningMode, _RunningModeDefault));
-
-
-
-
-int         SEventConfig::_StartIndex = ssys::getenvint(kStartIndex, _StartIndexDefault );
-int         SEventConfig::_NumEvent = ssys::getenvint(kNumEvent, _NumEventDefault );
-
-std::vector<int64_t>* SEventConfig::_GetNumPhotonPerEvent()
-{
-    const char* spec = ssys::getenvvar(kNumPhoton,  _NumPhotonDefault );
-    return sstr::ParseIntSpecList<int64_t>( spec, ',' );
-}
-std::vector<int64_t>* SEventConfig::_NumPhotonPerEvent = _GetNumPhotonPerEvent() ;
-
-
-std::vector<int64_t>* SEventConfig::_GetNumGenstepPerEvent()
-{
-    const char* spec = ssys::getenvvar(kNumGenstep,  _NumGenstepDefault );
-    return sstr::ParseIntSpecList<int64_t>( spec, ',' );
-}
-std::vector<int64_t>* SEventConfig::_NumGenstepPerEvent = _GetNumGenstepPerEvent() ;
-
-
-/**
-SEventConfig::_GetNumPhoton
----------------------------
-
-Used by some tests that define a sequence of photon counts as inputs::
-
-    OPTICKS_NUM_PHOTON=M1,2,3,4 OPTICKS_NUM_EVENT=4 SEventConfigTest
-    OPTICKS_NUM_PHOTON=G1,2,3,4 OPTICKS_NUM_EVENT=4 SEventConfigTest
-
-
-Expected to define the below of envvars with equal numbers of entries::
-
-   OPTICKS_NUM_PHOTON=M1,2,3,4
-   OPTICKS_NUM_GENSTEP=1,1,1,1
-
-
-**/
-int64_t SEventConfig::_GetNumPhoton(int idx)
-{
-    if(_NumPhotonPerEvent == nullptr) return 0 ;
-
-    int nevt0 = NumEvent() ;
-    int nevt1 = _NumPhotonPerEvent->size() ;
-    bool match = nevt0 == nevt1 ;
-    LOG_IF(fatal, !match)
-        << " NumEvent MISMATCH BETWEEN "
-        << std::endl
-        << " nevt0:_NumEvent               " << nevt0 << "( from " << kNumEvent  << ":" << ( getenv(kNumEvent) ? getenv(kNumEvent) : "-" ) << ") "
-        << " nevt1:_NumPhotonPerEvent.size " << nevt1 << "( from " << kNumPhoton << ":" << ( getenv(kNumPhoton) ? getenv(kNumPhoton) : "-" ) << ") "
-        ;
-    assert( match );
-    if(idx < 0 ) idx += nevt0 ;   // allow -ve indices to count from the back
-    if(idx >= nevt0) return 0 ;
-
-    return (*_NumPhotonPerEvent)[idx] ;
-}
-
-
-int64_t SEventConfig::_GetNumGenstep(int idx)
-{
-    if(_NumGenstepPerEvent == nullptr) return 0 ;
-
-    int nevt0 = NumEvent() ;
-    int nevt1 = _NumGenstepPerEvent->size() ;
-    bool match = nevt0 == nevt1 ;
-    LOG_IF(fatal, !match)
-        << " NumEvent MISMATCH BETWEEN "
-        << std::endl
-        << " nevt0:_NumEvent                " << nevt0 << "( from " << kNumEvent   << ":" << ( getenv(kNumEvent) ? getenv(kNumEvent) : "-" ) << ") "
-        << " nevt1:_NumGenstepPerEvent.size " << nevt1 << "( from " << kNumGenstep << ":" << ( getenv(kNumGenstep) ? getenv(kNumGenstep) : "-" ) << ") "
-        ;
-    assert( match );
-    if(idx < 0 ) idx += nevt0 ;   // allow -ve indices to count from the back
-    if(idx >= nevt0) return 0 ;
-
-    return (*_NumGenstepPerEvent)[idx] ;
-}
+int         SEventConfig::_RunningMode = SRM::Type(_RunningModeDefault);


 )

+target_compile_definitions(ConfigTest PRIVATE
+    SIMPHONY_SOURCE_CONFIG_DIR="${CMAKE_SOURCE_DIR}/config"


+    NP* ip = SEvt::GetInputPhoton(0);

-    LOG_IF(error, ip == nullptr) << "SEvt::GetInputPhoton GIVES nullptr : SET OPTICKS_INPUT_PHOTON TO CONFIGURE " ; 
+    LOG_IF(error, ip == nullptr) << "SEvt::GetInputPhoton GIVES nullptr : set event.input_photon in JSON";


Finish the event_mode rename across every shipped JSON profile and move the WLS event policy, bounce limit, and slot capacity out of its shell environment into wls_test.json. Remove redundant legacy overrides from Config-backed timing and photon-file workflows, pass an explicit JSON profile through the performance runner, and update the WLS output path for the simplified event_reldir default. Add a ConfigTest matrix covering the intended event policy of every shipped profile. JSON and script syntax checks pass, ConfigTest is green, and test_GPUPhotonFileSource.sh passes end to end; the WLS comparison skips because its external validator is unavailable.

Add typed Config fields for the retained SEventConfig runtime, resource, propagation, output, and input controls, parse them from the event object, and apply them through the existing low-level facade. Preserve useful OPTICKS_* settings as a one-release compatibility fallback when JSON keys are omitted, make explicit JSON values authoritative, and emit migration warnings that name the replacement keys. Extend ConfigTest to cover the complete JSON contract, adapter application, and precedence behavior. Also add the missing event-skipahead setter and non-lossy max-simtrace setter.

Stop loading obsolete, overwritten, or policy-derived OPTICKS_* values for event naming, per-event test counts, debug buffer maxima, legacy RNG/photon caps, save/client modes, compressed-record domains, ray-generation mode, component masks, and unused input selection. Keep the internal SEventConfig state required by existing allocation and programmatic callers, but derive component masks and debug maxima from event policy and default ray generation to simulate. Replace the disconnected event-name expansion with event.event_reldir. Add a fresh-process CTest that sets every retired variable and proves none affects runtime state. The complete enabled SysRap suite passes (101 tests; one pre-existing test disabled).

Reject event.mode explicitly instead of silently retaining the inherited or default event mode after the JSON schema rename. Point users to event.event_mode and cover the behavior in ConfigTest while leaving the valid torch.mode setting on its existing parser path.

Keep Config::output_dir at the current working directory when SEventConfig only contains its unresolved $DefaultOutputDir sentinel. Continue inheriting explicit JSON, environment, and programmatic output-directory values. Add regression coverage for omitted event.output_dir and document the distinction between application defaults and low-level SEvt path handling. This restores hit arrays and timing CSVs for the affected integration tests.

Make typed Config defaults plus the selected JSON file the sole source of modeled event settings, and apply every value deterministically through SEventConfig. Migrate the async and DD4hep examples to named JSON profiles, initialize DD4hep configuration once per process, and require C++20 for the plugin target. Remove retired variable names from code and documentation, update affected configuration files, and expand ConfigTest coverage for absent and empty event objects.

Resolve ConfigTest's source fixture directory relative to the SysRap test CMake file instead of the parent project's CMAKE_SOURCE_DIR. This keeps source-profile checks working when Simphony is included with add_subdirectory.

plexoos marked this pull request as ready for review July 6, 2026 22:20

Copilot AI review requested due to automatic review settings July 6, 2026 22:20

Copilot started reviewing on behalf of plexoos July 6, 2026 22:21

View session

plexoos force-pushed the rename-config-event-mode branch from 6ff6222 to b4c320f Compare July 6, 2026 22:21

plexoos self-assigned this Jul 6, 2026

plexoos added the refactor label Jul 6, 2026

chatgpt-codex-connector Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread sysrap/config.cpp

Copilot AI reviewed Jul 6, 2026

View reviewed changes

Comment thread sysrap/config.cpp

plexoos force-pushed the rename-config-event-mode branch from b4c320f to a270d5c Compare October 18, 2026 00:49

plexoos requested a balanced review from Copilot October 18, 2026 16:23

Copilot started reviewing on behalf of plexoos October 18, 2026 16:23

View session

Copilot AI reviewed Oct 18, 2026

View reviewed changes

plexoos changed the title ~~refactor(config): rename event mode key~~ refactor(config)!: make JSON authoritative for event settings Oct 18, 2026

plexoos added 7 commits October 18, 2026 19:52

fix(config): make test fixtures embedding-safe

cdab874

Resolve ConfigTest's source fixture directory relative to the SysRap test CMake file instead of the parent project's CMAKE_SOURCE_DIR. This keeps source-profile checks working when Simphony is included with add_subdirectory.

plexoos force-pushed the rename-config-event-mode branch from b524e77 to cdab874 Compare October 18, 2026 23:54

Navigation Menu

Repository navigation

refactor(config)!: make JSON authoritative for event settings - #401

refactor(config)!: make JSON authoritative for event settings#401
plexoos wants to merge 7 commits into
mainfrom
rename-config-event-mode

plexoos commented Jul 6, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

plexoos commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking changes

Migration

Scope

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

plexoos commented Jul 6, 2026 •

edited

Loading