Add timeframe splitting framework to EICrecon by 11kumaoka · Pull Request #2824 · eic/EICrecon

11kumaoka · 2026-08-03T21:19:25Z

Briefly, what does this PR introduce? Please link to any relevant presentations or discussions.

This PR introduces timeframe splitting functionality across EICrecon. Specifically, it add an optional splitting plugin which behaves as follows:

The plugin is ignored completely unless the split_timeframes parameter is set to true.
When enabled, the plugin adds a TimeframeSplitter component which partitions a Timeframe into zero or more PhysicsEvents.
When enabled, the plugin adds several factories for computing time alignments between detectors. These are run in parallel at the timeframe level before splitting happens.

To support timeframe splitting, the following changes had to be made across the detector plugins:

Some detector-level factories need to run upstream of splitting, particularly digitization.
JOmniFactoryGenerator now has an optional JEventLevel argument so that plugins may specify this.
For factories that need to run at the timeframe level, InitPlugin now checks the value of split_timeframes and sets the JEventLevel to Timeframe.
Several factories, notably ONNXInference_factory, throw an error when run at the timeframe level. Since a classical alternative factory exists, the ONNX factories have been temporarily disabled when split_timeframes=true. Fixing this is necessary in the medium term, but should be a separate work package.

Several additional changes have been made to the PODIO event source and processor:

JEventSource_Podio now has its event level set to Timeframe when split_timeframes=true. Note that the decision about whether to do timeframe splitting only depends on that flag, and not on the input file itself. Logic for examining the input file metadata can be added at a later date.
JEventProcessor_Podio gains two additional optional input collections, EventHeader_PHY and EventHeader_BKG. These indicate that the respective trigger fired inside TimeframeSplitter::Unfold.

What is the urgency of this PR?

High (please describe reason below)
Medium
Low

What kind of change does this PR introduce?

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.
AI-assisted coding tools were used to help compare the previous implementation with the current EICrecon APIs, identify migration-related compilation and runtime issues, and prepare portions of the code changes. The resulting changes were reviewed and tested by the author.

Copilot

Pull request overview

Introduces a new “timeframe splitting” framework to allow processing Timeslice-level data and unfolding it into PhysicsEvent-level events, wiring this through PODIO input/output and a new splitting plugin, plus detector/plugin updates to run selected factories at JEventLevel::Timeslice when split_timeframes=1.

Changes:

Adds a new splitting plugin with time-alignment factories and a JEventUnfolder (TimeframeSplitter) to split timeframes into physics events.
Plumbs a split_timeframes parameter into PODIO and multiple detector plugins to switch factory/event-source levels between PhysicsEvent and Timeslice.
Extends JOmniFactoryGeneratorT to allow specifying a factory’s JEventLevel at construction time.

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/utilities/eicrecon/eicrecon.cc	Adds `splitting` to the default plugin list.
src/services/io/podio/podio.cc	Adds `split_timeframes` parameter and sets PODIO event-source level based on it.
src/services/io/podio/JEventProcessorPODIO.cc	Extends default output collection list when timeframe splitting is enabled.
src/global/splitting/TrkTimeAlignmentFactory.h	New tracker-hit time alignment OmniFactory producing `_aligned` collections.
src/global/splitting/CalRecTimeAlignmentFactory.h	New calorimeter rec-hit time alignment OmniFactory producing `_aligned` collections.
src/global/splitting/TimeframeSplitter.h	New `JEventUnfolder` implementation to unfold Timeslice → PhysicsEvent and rebuild relations.
src/global/splitting/splitting.cc	New plugin entrypoint registering alignment factories and the unfolder.
src/global/splitting/CMakeLists.txt	Adds build rules for the new `splitting` plugin (conditionally on JANA version).
src/global/CMakeLists.txt	Adds `splitting` subdirectory to the global build.
src/extensions/jana/JOmniFactoryGeneratorT.h	Adds support for setting factory `JEventLevel` via generator wiring/constructors.
src/detectors/ZDC/ZDC.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.
src/detectors/RPOTS/RPOTS.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.
src/detectors/MPGD/MPGD.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.
src/detectors/LUMISPECCAL/LUMISPECCAL.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.
src/detectors/LOWQ2/LOWQ2.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.
src/detectors/FOFFMTRK/FOFFMTRK.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.
src/detectors/FHCAL/FHCAL.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.
src/detectors/FEMC/FEMC.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.
src/detectors/EHCAL/EHCAL.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.
src/detectors/EEMC/EEMC.cc	Adjusts clustering/PID chain behavior when splitting is enabled; sets Timeslice vs PhysicsEvent hit levels.
src/detectors/ECTRK/ECTRK.cc	Disables noise-overlay path when splitting; sets Timeslice vs PhysicsEvent hit levels.
src/detectors/ECTOF/ECTOF.cc	Adds Timeslice digi/reco path when splitting; adjusts downstream input selection.
src/detectors/BVTX/BVTX.cc	Disables noise-overlay path when splitting; sets Timeslice vs PhysicsEvent hit levels.
src/detectors/BTRK/BTRK.cc	Disables noise-overlay path when splitting; sets Timeslice vs PhysicsEvent hit levels.
src/detectors/BTOF/BTOF.cc	Adds Timeslice digi/reco path when splitting; adjusts downstream input selection.
src/detectors/BHCAL/BHCAL.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.
src/detectors/BEMC/BEMC.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.
src/detectors/B0TRK/B0TRK.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.
src/detectors/B0ECAL/B0ECAL.cc	Uses `split_timeframes` to choose Timeslice vs PhysicsEvent factory level for hit digi/reco.

Suppressed comments (4)

src/global/splitting/TrkTimeAlignmentFactory.h:57

Process() prints to std::cout every event, which will severely impact throughput and flood logs in multithreaded production runs. Prefer using the framework logger at a debug level (or removing the print entirely).

  void Process(int64_t run_number, uint64_t event_number) {
    std::cout << "<<<<<<<<<<<<Time Alignment Factory: Event " << event_number << std::endl;
    unsigned int nColls = 0;

src/global/splitting/TrkTimeAlignmentFactory.h:87

This loop copies each MutableTrackerHit (for (auto hit : sorted_hits)) and then computes hitTime without using it. Iterating by const reference avoids extra copies and dropping the unused variable avoids warnings/overhead.

        for (auto hit : sorted_hits) {
          auto hitTime = hit.getTime();
          coll_out->push_back(hit);
        }

src/global/splitting/CalRecTimeAlignmentFactory.h:57

Process(int64_t run_number, uint64_t event_number) doesn't use either argument; naming them can trigger -Wunused-parameter warnings in some build configurations. Consider commenting them out in the signature if they are intentionally unused.

  void Process(int64_t run_number, uint64_t event_number) {

src/global/splitting/CalRecTimeAlignmentFactory.h:82

This loop copies each MutableCalorimeterHit (for (auto hit : sorted_hits)) and then computes hitTime without using it. Iterating by const reference avoids extra copies and removes the unused variable.

        for (auto hit : sorted_hits) {
          auto hitTime = hit.getTime();
          coll_out->push_back(hit);
        }

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

Copilot

Pull request overview

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

Suppressed comments (6)

src/utilities/eicrecon/eicrecon.cc:53

splitting is added to the default plugin list unconditionally, but the splitting plugin can be skipped at build time when JANA_VERSION < 2.4.3 (see src/global/splitting/CMakeLists.txt). In that configuration the executable will still try to load a plugin that was never built/installed, which can cause startup failure.

Consider removing splitting from the unconditional defaults, or gating it behind a compile-time/version check so it is only included when the plugin is guaranteed to exist.

    "splitting",

src/global/splitting/TimeframeSplitter.h:47

The parameter keys for the time resolution settings include spaces and = (e.g. "timeResolution_MPGD = 10.0"), and the embedded = 10.0/= 1.0 text doesn't match the actual defaults (30.0/20.0). This makes the configuration keys awkward to set from the CLI/config and makes the help text misleading.

Use stable, whitespace-free keys (e.g. timeResolution_MPGD, timeResolution_TOF, timeResolution_EMCal) and keep the help strings consistent with the defaults.

  Parameter<float> timeResolution_SiMaps{this, "timeResolution_Silicon", 2000.0,
                                         "time resolution of Silicon detector in ns"};
  Parameter<float> timeResolution_MPGD{this, "timeResolution_MPGD", 30.0,
                                       "time resolution of MPGD detector in ns"};
  // Parameter<float> timeResolution_ACLGad{this, "timeResolution_TOF", 0.03,
  //                                     "time resolution of TOF detector in ns"};
  Parameter<float> timeResolution_ACLGad{this, "timeResolution_TOF", 20.0,
                                         "time resolution of TOF detector in ns"};
  Parameter<float> timeResolution_EMCal{this, "timeResolution_EMCal", 20.0,
                                        "time resolution of EMCal detector in ns"};

src/global/splitting/TimeframeSplitter.h:721

copy_tracker_hit_with_relations scans the entire associations collection for every tracker hit to find matching raw-hit IDs. This creates O(N_hits * N_associations) behavior per event and can become a significant bottleneck for high-occupancy timeframes.

Consider pre-indexing the associations by RawHit ObjectID (e.g. building a multimap once per event) and then looking up the subset that matches the current raw hit.

    for (const auto& association : *associations) {
      const auto association_raw_hit = association.getRawHit();
      if (!association_raw_hit.isAvailable() || association_raw_hit.getObjectID() != raw_hit_id) {
        continue;
      }
      if (!association.getSimHit().isAvailable())
        continue;

src/global/splitting/TrkTimeAlignmentFactory.h:21

The factory type name timeAlignmentFactory doesn't match the established *_factory naming used by existing JOmniFactory implementations throughout the codebase (e.g. CalorimeterHitReco_factory, TrackerHitReconstruction_factory). This makes it harder to search/grep and is inconsistent with the rest of the factory ecosystem.

Consider renaming it to something like TimeAlignment_factory (and updating the JOmniFactoryGeneratorT<...> instantiation accordingly).

struct timeAlignmentFactory : public JOmniFactory<timeAlignmentFactory> {

src/global/splitting/TrkTimeAlignmentFactory.h:63

Process currently writes to std::cout for every event and also declares unused variables (nColls, plus the run_number parameter). In multi-threaded JANA execution this will severely spam logs and can become a synchronization bottleneck; the unused variables may also trigger warnings.

If logging is needed, prefer a JANA logger with appropriate verbosity; otherwise remove the std::cout and unused variables.

  void Process(int64_t run_number, uint64_t event_number) {
    std::cout << "<<<<<<<<<<<<Time Alignment Factory: Event " << event_number << std::endl;
    unsigned int nColls = 0;
    for (size_t coll_index = 0; coll_index < m_trackerhits_in().size(); ++coll_index) {

src/global/splitting/CalRecTimeAlignmentFactory.h:87

This loop introduces an unused local (hitTime) which can trigger compiler warnings and makes the code noisier to read. It can be removed without changing behavior.

        for (auto hit : sorted_hits) {
          auto hitTime = hit.getTime();
          coll_out->push_back(hit);

Copilot

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.

Suppressed comments (6)

src/global/splitting/TrkTimeAlignmentFactory.h:62

nColls is never used; with -Werror this will fail compilation. Remove it (and the corresponding increment) or otherwise use it.

    unsigned int nColls = 0;

src/global/splitting/TrkTimeAlignmentFactory.h:94

This increment is dead code once nColls is removed, and will also fail compilation if nColls is commented out/removed above.

      nColls++;

src/utilities/eicrecon/eicrecon.cc:54

splitting is added to the default plugin list, but the plugin build is explicitly skipped when JANA_VERSION < 2.4.3 (see src/global/splitting/CMakeLists.txt). In those configurations, eicrecon will still try to load splitting by default, which can cause startup failure due to a missing plugin.

    "ECTOF",
    "LOWQ2",
    "LUMISPECCAL",
    "splitting",
    "podio",

src/global/splitting/TrkTimeAlignmentFactory.h:92

hitTime is unused, which fails CI under -Werror. Also, iterating by value (for (auto hit : ...)) copies each element unnecessarily.

        for (auto hit : sorted_hits) {
          auto hitTime = hit.getTime();
          coll_out->push_back(hit);
        }

src/global/splitting/CalRecTimeAlignmentFactory.h:87

hitTime is unused, which fails CI under -Werror. Also, iterating by value (for (auto hit : ...)) copies each element unnecessarily.

        for (auto hit : sorted_hits) {
          auto hitTime = hit.getTime();
          coll_out->push_back(hit);
        }

src/global/splitting/CMakeLists.txt:24

plugin_add_onnxruntime(${PLUGIN_NAME}) adds an ONNX Runtime dependency, but there is no ONNX/onnxruntime usage anywhere in this plugin directory. Keeping this dependency can unnecessarily constrain build environments.

  plugin_add_event_model(${PLUGIN_NAME})
  plugin_add_onnxruntime(${PLUGIN_NAME})

Copilot

Pull request overview

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

Suppressed comments (6)

src/global/splitting/TimeframeSplitter.h:801

m_mcparticles_in is used without calling its accessor (*m_mcparticles_in), but later in the same function it is used as *m_mcparticles_in(). As written this won’t compile (and is inconsistent with the other loop).

      Double_t prevMCTime = -9999.0; // temp check mc particle times
      for (const auto& mcparticle : *m_mcparticles_in) {
        // if (mcparticle.parents_begin() != 0)

src/global/splitting/TrkTimeAlignmentFactory.h:62

Unconditional std::cout logging in Process() will spam output and significantly slow down processing, especially in multi-threaded runs. Prefer using the framework logger at an appropriate level (or guard behind a debug/QA parameter).


  void Process(int64_t /*run_number*/, uint64_t event_number) {
    std::cout << "<<<<<<<<<<<<Time Alignment Factory: Event " << event_number << std::endl;

src/global/splitting/CMakeLists.txt:24

plugin_add_onnxruntime(${PLUGIN_NAME}) pulls in an extra dependency, but there are no ONNX/ONNXRuntime references in this plugin’s sources. This adds avoidable build/dependency weight for the splitting plugin.

  plugin_add_event_model(${PLUGIN_NAME})

  # Add include directories (works same as target_include_directories)

src/global/splitting/TimeframeSplitter.h:1359

child_idx is passed by value to Unfold(...), so incrementing it here has no effect on the caller and is confusing. The return value (Result::NextChildKeepParent) already expresses the intended control flow.

    } else if (m_bTrigger) {
      child_idx++;
      return Result::NextChildKeepParent;

src/global/splitting/TrkTimeAlignmentFactory.h:11

This header uses ROOT typedefs like Double_t and writes to std::cout, but it doesn’t include headers that define them (<RtypesCore.h> for Double_t and <iostream> for std::cout). This is brittle and can fail to compile depending on include order.

This issue also appears on line 60 of the same file.

#include <algorithm>
#include <cmath>
#include <iostream>
#include <string>
#include <vector>

src/global/splitting/CalRecTimeAlignmentFactory.h:11

This header uses ROOT typedefs like Double_t but doesn’t include headers that define them. Relying on indirect includes is fragile and can fail to compile depending on include order.

#include <algorithm>
#include <cmath>
#include <string>
#include <vector>

Copilot

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.

Suppressed comments (6)

src/global/splitting/TrkTimeAlignmentFactory.h:92

hitTime is assigned but never used, which can trigger -Wunused-but-set-variable warnings and obscures intent.

        for (auto hit : sorted_hits) {
          auto hitTime = hit.getTime();
          coll_out->push_back(hit);

src/global/splitting/CalRecTimeAlignmentFactory.h:86

hitTime is assigned but never used, which can trigger -Wunused-but-set-variable warnings and obscures intent.

        for (auto hit : sorted_hits) {
          auto hitTime = hit.getTime();
          coll_out->push_back(hit);

src/global/splitting/TimeframeSplitter.h:721

copy_tracker_hit_with_relations() linearly scans the full associations collection for every tracker hit. This is O(N_hits × N_associations) per detector and can become a major bottleneck for large timeframes. Consider pre-indexing associations by raw-hit ObjectID once per detector/timeframe (e.g. unordered_multimap<ObjectID, const Assoc*>) and then only iterating the matching range for each hit.

    for (const auto& association : *associations) {
      const auto association_raw_hit = association.getRawHit();
      if (!association_raw_hit.isAvailable() || association_raw_hit.getObjectID() != raw_hit_id) {
        continue;
      }
      if (!association.getSimHit().isAvailable())
        continue;

src/global/splitting/TrkTimeAlignmentFactory.h:63

Writing to stdout inside Process() will spam logs and can significantly slow down multi-threaded production jobs. If this is needed for debugging, route it through the factory logger at debug level, or remove it and mark the parameter unused.

  void Process(int64_t /*run_number*/, uint64_t event_number) {
    std::cout << "<<<<<<<<<<<<Time Alignment Factory: Event " << event_number << std::endl;
    unsigned int nColls = 0;

src/global/splitting/splitting.cc:80

The calorimeter cluster name vectors are unused because the corresponding CalTimeAlignmentFactory wiring is commented out. Keeping these unused locals can cause warnings and makes it unclear what is actually required by the plugin.

  std::vector<std::string> m_simcalocluster_collection_names_aligned = {
      "B0ECalClusters_TK_aligned", "EcalBarrelClusters_TK_aligned",
      "EcalEndcapNClusters_TK_aligned", "EcalEndcapPClusters_TK_aligned"};
  // "EcalFarForwardZDCClusters_TK_aligned",
  //   "EcalLumiSpecClusters_TK_aligned",
  //   "HcalBarrelClusters_TK_aligned",
  //   "HcalEndcapNClusters_TK_aligned",
  //   "HcalEndcapPInsertClusters_TK_aligned",
  //   "HcalFarForwardZDCClusters_TK_aligned",
  //   "LFHCALClusters_TK_aligned"

  std::vector<std::string> m_simcalocluster_collection_names = {
      "B0ECalClusters_TK", "EcalBarrelClusters_TK", "EcalEndcapNClusters_TK",
      "EcalEndcapPClusters_TK"};

src/global/splitting/CMakeLists.txt:1

cmake_minimum_required() should be set at the top-level CMakeLists.txt, not repeated in subdirectories (it can unexpectedly reset policies for the subdir).

cmake_minimum_required(VERSION 3.16)

Copilot

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (6)

src/global/splitting/TimeframeSplitter.h:801

m_mcparticles_in is a PodioInput functor and must be invoked with (). As written, *m_mcparticles_in is not valid C++ and will fail to compile.

      Double_t prevMCTime = -9999.0; // temp check mc particle times
      for (const auto& mcparticle : *m_mcparticles_in) {
        // if (mcparticle.parents_begin() != 0)

src/global/splitting/TimeframeSplitter.h:1359

child_idx is passed by value to Unfold, so incrementing it here has no effect and is misleading. It should be removed.

    } else if (m_bTrigger) {
      child_idx++;
      return Result::NextChildKeepParent;
    }

src/global/splitting/TrkTimeAlignmentFactory.h:64

Avoid writing directly to std::cout inside Process (it is expensive and interleaves across threads). Use the factory logger instead; also mark the per-event counter as intentionally unused to avoid unused-but-set warnings.

    std::cout << "<<<<<<<<<<<<Time Alignment Factory: Event " << event_number << std::endl;
    unsigned int nColls = 0;

src/global/splitting/TrkTimeAlignmentFactory.h:94

This loop copies each hit (for (auto hit : ...)) and computes hitTime which is never used. Prefer iterating by const-reference and remove the unused variable.

        for (auto hit : sorted_hits) {
          auto hitTime = hit.getTime();
          coll_out->push_back(hit);
        }

src/global/splitting/CalRecTimeAlignmentFactory.h:88

hitTime is computed but never used, and the loop copies each hit. Prefer iterating by const-reference and remove the unused variable.

        for (auto hit : sorted_hits) {
          auto hitTime = hit.getTime();
          coll_out->push_back(hit);
        }

src/global/splitting/CMakeLists.txt:7

cmake_minimum_required() is typically only called at the project top-level; calling it in a subdirectory can reset CMake policy scopes unexpectedly. Also, guard the version comparison in case JANA_VERSION is not defined yet.

cmake_minimum_required(VERSION 3.16)

if(JANA_VERSION VERSION_LESS "2.4.3")

  message(STATUS "Skipping `splitting` plugin because JANA_VERSION < 2.4.3")

else()

Copilot

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

src/global/splitting/TimeframeSplitter.h:800

m_mcparticles_in is a PodioInput and must be accessed via operator(). As written (*m_mcparticles_in) this won’t compile because PodioInput doesn’t define operator*.

      for (const auto& mcparticle : *m_mcparticles_in) {

src/services/io/podio/JEventProcessorPODIO.cc:534

EventHeader_PHY / EventHeader_BKG are always included in the default include-list, but they are only produced when timeframe splitting is enabled. When splitting is off, FindCollectionsToWrite will warn that these explicitly included collections are missing. Consider only adding them when split_timeframes is true.

      "EventHeader_PHY",
      "EventHeader_BKG",

wdconinc · 2026-08-13T00:36:49Z

+      sorted_hits.push_back(copied_hit);
+    }
+
+    std::stable_sort(sorted_hits.begin(), sorted_hits.end(), [](const auto& lhs, const auto& rhs) {


Just in case someone has the same thought, the size of a hit here is typically < 64 bytes so a hit fits in an L1 cache line and so there is no benefit to sort an iota to avoid the move costs. Microbenchmark could be useful but not necessary since likely not a major impact on performance here at this point.

wdconinc · 2026-08-13T00:42:07Z

+    for (std::size_t index = 0; index < m_hits_in().size(); ++index) {
+      const auto* hits_in = m_hits_in().at(index);
+      if (hits_in != nullptr) {
+        m_algo->process({hits_in}, {m_hits_out().at(index).get()});


This misses a size check in the output. A different number of input and outputs is not flagged. However, I don't think this should be variadic. It is (somewhat a matter of opinion) better to keep this focused on a single collection as input and single collection as output.

wdconinc · 2026-08-13T00:44:04Z

+    return;
+  }
+
+  app->Add(new JOmniFactoryGeneratorT<eicrecon::RecHitTimeAlignment_factory<edm4eic::TrackerHit>>(


Rather than passing a long list of collections as input and output, I'd suggest creating many single collection factories.

wdconinc · 2026-08-13T00:44:38Z

+      "timeAlignment", m_simtrackerhit_collection_names, m_simtrackerhit_collection_names_aligned,
+      app, JEventLevel::Timeslice));
+
+  app->Add(


And same here.

wdconinc · 2026-08-13T00:50:52Z

+  // == Global Variables =======================
+  bool bInitialLoop = true;
+
+  Int_t m_multiTriggerThreshold[4] = {1, 4, 20, 20};


The use of ROOT types stands out as a change from common practice elsewhere in this PR and code base. I'd suggest to stay with standard C++ types instead of their ROOT counterparts and to use inherently unsigned versions where appropriate, such as here in the case of the number of hits (inherently positive). In that case, use unsigned int. For indices, use std::size_t.

wdconinc · 2026-08-13T00:54:33Z

+  static std::uint64_t object_id_key(const podio::ObjectID& object_id);
+
+  static TrackerAssociationIndex
+  buildTrkAssoId(const edm4eic::MCRecoTrackerHitAssociationCollection* associations);


This code switches between snake_case and dromedaryCase for functions. I'd suggest picking the one that's most commonly used in EICrecon and JANA2 (probably dromedaryCase) and using it consistently throughout.

wdconinc · 2026-08-13T00:57:32Z

+    const Double_t hitX = hit.getPosition()[0];
+    const Double_t hitY = hit.getPosition()[1];
+    const Double_t hitZ = hit.getPosition()[2];
+    const Double_t hitR = TMath::Sqrt(hitX * hitX + hitY * hitY + hitZ * hitZ);


No need to use TMath anymore.

wdconinc · 2026-08-13T01:01:58Z

CI canceled. Deadlocked at https://github.com/eic/EICrecon/actions/runs/31636240906/job/94259341005?pr=2824#step:7:671.

This hang is reproducible locally. Like a matter of the infinite loop not exiting. Any infinite loop design benefits from explicit comments documenting how it will terminate (or a termination proof, haha).

for more information, see https://pre-commit.ci

…_dev

for more information, see https://pre-commit.ci

wdconinc · 2026-08-13T16:09:15Z

A regression visible in the single-threaded CI running as well, when comparing the new eicrecon-timeframe-splitting (clang++, 18x275, 100, craterlake_18x275, ASAN) vs baseline eicrecon-dis (clang++, 18x275, 100, craterlake_18x275, ASAN). In the new run (on the same input data), we are missing a lot of collections:

  Error: [e:0] [JEventProcessorPODIO] [error] Omitting PODIO collection 'DIRCRawHits' due to exception: JOmniFactory: Failed to get collection DIRCBarHits: Missing databundle with uniquename 'DIRCBarHits'.
  Error: [e:0] [JEventProcessorPODIO] [error] Omitting PODIO collection 'DRICHAerogelIrtCherenkovParticleID' due to exception: JOmniFactory: Failed to get collection DRICHRawHits: JOmniFactory: Failed to get collection DRICHHits: Missing databundle with uniquename 'DRICHHits'.
  Error: [e:0] [JEventProcessorPODIO] [error] Omitting PODIO collection 'DRICHGasIrtCherenkovParticleID' due to exception: JOmniFactory: Failed to get collection DRICHRawHits: JOmniFactory: Failed to get collection DRICHHits: Missing databundle with uniquename 'DRICHHits'.
  Error: [e:0] [JEventProcessorPODIO] [error] Omitting PODIO collection 'DRICHRawHits' due to exception: JOmniFactory: Failed to get collection DRICHHits: Missing databundle with uniquename 'DRICHHits'.
  Error: [e:0] [JEventProcessorPODIO] [error] Omitting PODIO collection 'DRICHRawHitsAssociations' due to exception: JOmniFactory: Failed to get collection DRICHHits: Missing databundle with uniquename 'DRICHHits'.
  Error: [e:0] [JEventProcessorPODIO] [error] Omitting PODIO collection 'DRICHRawHitsLinks' due to exception: JOmniFactory: Failed to get collection DRICHHits: Missing databundle with uniquename 'DRICHHits'.

There are no such errors reported in the baseline running.

wdconinc · 2026-08-13T17:20:03Z

+template <typename SourceT>
+class LeveledEventSourceGeneratorT : public JEventSourceGeneratorT<SourceT> {
+public:
+  explicit LeveledEventSourceGeneratorT(JEventLevel level) { this->SetLevel(level); }
+};


Instead of defining a modified source generator here in this plugin, I think it makes more sense to put this in JANA2. @nathanwbrei Is there no way to define the level of an event source in JANA2 with just JEventSourceGeneratorT?

Right now the setter is protected, so everyone has to extend JEventSourceGeneratorT, which is exactly what this thing does. I can make it no longer protected in the next version.

wdconinc · 2026-08-13T17:25:21Z

A regression visible in the single-threaded CI running as well, when comparing the new eicrecon-timeframe-splitting (clang++, 18x275, 100, craterlake_18x275, ASAN) vs baseline eicrecon-dis (clang++, 18x275, 100, craterlake_18x275, ASAN).

Some more digging reveals that this is simply because these PID detectors are not included in the time splitter yet, so they never make it from the time slice level to the physics event level. Not sure there's another solution to this than just adding support for the PID detectors in TimeSplitter.

This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/31672030046. Please merge this PR into the branch `timeframe_splitting_dev` to resolve failures in PR #2824. Auto-generated by [create-pull-request][1] [1]: https://github.com/peter-evans/create-pull-request Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/utilities/eicrecon/eicrecon.cc:55

The default plugin list enables the "splitting" plugin for any JANA major version >= 2026, but the build only adds the splitting subdirectory when JANA_VERSION >= 2026.01.00. If a 2026.00.* release exists, this would try to load a plugin that was not built/installed. Consider matching the compile-time guard to the same (major, minor) threshold used by CMake.
src/extensions/jana/JOmniFactoryGeneratorT.h:22
JOmniFactoryGeneratorT now stores/uses JEventLevel, but this header does not include the definition for JEventLevel. Relying on transitive includes is brittle and can break compilation for translation units that include this header without already including JEventLevel.

  struct TypedWiring {
    std::string m_tag;
    std::vector<std::string> m_default_input_tags;
    std::vector<std::string> m_default_output_tags;
    FactoryConfigType m_default_cfg; /// Must be properly copyable!
    JEventLevel m_level = JEventLevel::PhysicsEvent;
  };

src/factories/event_building/HitTimeAlignment_factory.h:2

Typo in the file header comment: "codCopyright" should be "Copyright".

// codCopyright (C) 2026 Takuya Kumaoka

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

for more information, see https://pre-commit.ci

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

for more information, see https://pre-commit.ci

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

veprbl

Some initial comments. Also, this appears to currently lock up at event 4 on CI.

(didn't mean to approve, just commenting for now)

veprbl · 2026-08-14T13:51:41Z

+  // Per-pixel noise occupancy for the barrel silicon tracker. Configurable via
+  // SiBarrelNoiseRawHits:noise_rate_per_pixel_per_event (default 2e-7).


Suggested change

// Per-pixel noise occupancy for the barrel silicon tracker. Configurable via

// SiBarrelNoiseRawHits:noise_rate_per_pixel_per_event (default 2e-7).

Incorrect rebase.

veprbl · 2026-08-14T14:05:05Z

+#include <JANA/Components/JComponent.h>
+#include <JANA/Components/JPodioOutput.h>
+#include <JANA/JEventUnfolder.h>
+#include <RtypesCore.h>


This should not be needed if we don't use Double_t, Int_t, etc

veprbl · 2026-08-14T16:48:10Z

+    else
+      return Result::KeepChildNextParent;
+  } else if (m_bTrigger) {
+    child_idx++;


Suggested change

child_idx++;

Has no effect for value in the end of the function.

veprbl · 2026-08-14T16:50:13Z

+
+    if (physEventWeight == 1) {
+      edm4hep::MutableEventHeader eventHeader_bkg;
+      eventHeader_bkg.setRunNumber(m_eventNumber_TS * 10000 + child_idx);


Should we print a warning about index overrun whenever child_idx >= 10000

veprbl · 2026-08-14T16:51:10Z

+  unsigned int m_PhysCount     = 0; //QA
+
+  size_t m_eventNumber_TS = 0;              // Event number for the current timeslice
+  std::vector<unsigned int> m_vTargetEvent; // List of original event numbers for each timeslice


Could you check for unused variables like this one.

veprbl · 2026-08-14T16:58:42Z

+    needs:
+    - build
+    - npsim-dis
+    - npsim-minbias


veprbl · 2026-08-14T17:02:10Z

+        if (judgeHitInTimeSlice(hitTime, timeResolution_emcal, tsTimeS, tsTimeE)) {
+          totalZDCEnergy += hit.getEnergy();
+          totalZDCEnergyTime += hit.getEnergy() * hitTime;
+          iniCalHitPoint[kCalZDC] = iHit;


I don't get how this works. You update it to a hit index, but it's also used in the loop initialization. Should this be first matching hit?

veprbl · 2026-08-14T17:06:15Z

+    if (bMutipliTriggers[0] || bMutipliTriggers[1] || bMutipliTriggers[2] || bMutipliTriggers[3] ||
+        bMutipliTriggers[4] || bMutipliTriggers[5])
+      bTimesliceTrigger =
+          true; // ???? temporary, need to be removed after geometrical coincidence trigger is implemented


Time to remove?

veprbl · 2026-08-14T17:08:30Z

+
+  unsigned int targetDetId            = 0;
+  size_t iTimeSlice                   = 0;
+  std::vector<double> m_vPhysCooTimes = {};


What is Coo?

veprbl · 2026-08-14T17:10:07Z

+                    backEndIntTimesEtaPhi, backEndIntTimesEtaPhiShifted, backEndEtaPhiBins);
+    singleTrig[0] =
+        countGridCellsWithMultiplicity(backEndCalGrid, backEndCalGridShifted, backEndIntTimesEtaPhi,
+                                       backEndIntTimesEtaPhiShifted, 10, singleTrigTime[0]);


10 is a magic number? can we have this and others like it as a named constant or even JANA2 parameter?

Copilot AI lite review requested due to automatic review settings August 3, 2026 21:19

Copilot started reviewing on behalf of 11kumaoka August 3, 2026 21:25 View session