feat(geometry): add non-throwing readout API and producer-side reduced-geometry gating - #2783
feat(geometry): add non-throwing readout API and producer-side reduced-geometry gating#2783wdconinc wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves robustness when running EICrecon with reduced-geometry detector configurations by (1) introducing a non-throwing DD4hep readout lookup API and (2) gating multiple geometry-dependent producers so missing readouts disable the producer (with a single warning) instead of throwing during initialization.
Changes:
- Added header-only
eicrecon::geohelpers (hasReadout,readoutIdSpec,readoutSegmentation) to avoid DD4hep exceptions for absent readouts. - Updated multiple producers across calorimetry, digi, tracking, and far-detectors to detect missing readouts at
init()time and early-return fromprocess()when disabled. - Added Catch2 unit tests covering the new non-throwing geometry helper behavior.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/algorithms/interfaces/GeometryUtils.h | Adds non-throwing readout presence/lookup helpers returning bool/std::optional. |
| src/tests/algorithms_test/interfaces_GeometryUtils.cc | Adds unit tests validating helpers don’t throw on missing readouts. |
| src/tests/algorithms_test/CMakeLists.txt | Registers the new GeometryUtils test in the algorithms test executable. |
| src/algorithms/tracking/MPGDHitReconstruction.h | Adds m_readout_available gate. |
| src/algorithms/tracking/MPGDHitReconstruction.cc | Disables algorithm on missing readout; early-returns in process(). |
| src/algorithms/tracking/LGADHitClustering.h | Adds m_readout_available gate. |
| src/algorithms/tracking/LGADHitClustering.cc | Disables algorithm on missing readout; early-returns in process(). |
| src/algorithms/fardetectors/FarDetectorTrackerCluster.h | Adds m_readout_available gate. |
| src/algorithms/fardetectors/FarDetectorTrackerCluster.cc | Disables algorithm on missing readout; early-returns in process(). |
| src/algorithms/digi/SiliconChargeSharing.h | Adds m_readout_available gate. |
| src/algorithms/digi/SiliconChargeSharing.cc | Disables algorithm on missing readout; early-returns in process(). |
| src/algorithms/digi/PulseCombiner.h | Adds m_readout_available gate. |
| src/algorithms/digi/PulseCombiner.cc | Disables algorithm on missing readout; early-returns in process(); clarifies init conditions. |
| src/algorithms/digi/MPGDTrackerDigi.h | Adds m_readout_available gate. |
| src/algorithms/digi/MPGDTrackerDigi.cc | Disables algorithm on missing readout; early-returns in process(). |
| src/algorithms/calorimetry/SimCalorimeterHitProcessor.h | Adds m_readout_available gate. |
| src/algorithms/calorimetry/SimCalorimeterHitProcessor.cc | Disables algorithm on missing readout; early-returns in process(). |
| src/algorithms/calorimetry/CalorimeterIslandCluster.h | Adds m_readout_available gate. |
| src/algorithms/calorimetry/CalorimeterIslandCluster.cc | Disables algorithm on missing readout; early-returns in process(). |
| src/algorithms/calorimetry/CalorimeterHitsMerger.h | Adds m_readout_available gate. |
| src/algorithms/calorimetry/CalorimeterHitsMerger.cc | Disables algorithm on missing/invalid decoder; early-returns in process(). |
| src/algorithms/calorimetry/CalorimeterHitDigi.h | Adds m_readout_available gate. |
| src/algorithms/calorimetry/CalorimeterHitDigi.cc | Disables algorithm on missing readout; early-returns in process(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…d-geometry gating (fix: iwyu) (#2784) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/30136636279. Please merge this PR into the branch `nonthrowing-geometry-readout-api` to resolve failures in PR #2783. 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>
| SECTION("present readout with a segmentation yields a valid segmentation") { | ||
| const auto segmentation = eicrecon::geo::readoutSegmentation(*detector, "MockTrackerHits"); | ||
| REQUIRE(segmentation.has_value()); | ||
| REQUIRE(segmentation->isValid()); | ||
| } | ||
|
|
||
| SECTION("absent readout yields nullopt without throwing") { | ||
| REQUIRE_NOTHROW(eicrecon::geo::readoutSegmentation(*detector, "NonexistentHits")); | ||
| REQUIRE_FALSE(eicrecon::geo::readoutSegmentation(*detector, "NonexistentHits").has_value()); | ||
| } |
| const auto missing_readout_policy = | ||
| eicrecon::geo::parseMissingReadoutPolicy(m_cfg.missingReadoutPolicy); | ||
| if (m_cfg.readout.empty()) { | ||
| if ((!m_cfg.adjacencyMatrix.empty()) || (!m_cfg.peakNeighbourhoodMatrix.empty())) { | ||
| throw std::runtime_error( | ||
| "'readout' is not provided, it is needed to know the fields in readout ids"); | ||
| } | ||
| } else { | ||
| if (!eicrecon::geo::hasReadout(*m_detector, m_cfg.readout)) { |
| void LGADHitClustering::process(const LGADHitClustering::Input& input, | ||
| const LGADHitClustering::Output& output) const { | ||
| const auto [calibrated_hits] = input; | ||
| if (!m_readout_available) { | ||
| return; | ||
| } |
…d-geometry gating (fix: iwyu) (#2785) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/30137354767. Please merge this PR into the branch `nonthrowing-geometry-readout-api` to resolve failures in PR #2783. 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>
| if (m_cfg.readout.empty()) { | ||
| error("readoutClass is not provided, it is needed to know the fields in readout ids"); | ||
| m_readout_available = false; | ||
| return; | ||
| } |
Capybara summary for PR 2783
Last updated 2026-08-12T18:33-04:00 a37a113 |
…d-geometry gating (fix: iwyu) (#2784) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/30136636279. Please merge this PR into the branch `nonthrowing-geometry-readout-api` to resolve failures in PR #2783. 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>
…d-geometry gating (fix: iwyu) (#2785) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/30137354767. Please merge this PR into the branch `nonthrowing-geometry-readout-api` to resolve failures in PR #2783. 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>
3dbd240 to
b3744d0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/algorithms/calorimetry/CalorimeterHitsMerger.cc:49
- When
readoutis empty,init()currently logs an error and silently disables the algorithm (m_readout_available = false). Unlike the other readout-dependent producers in this PR, this can mask a genuine configuration error and lead to missing outputs without failing the job. Consider throwing on an empty readout (the non-throwing behavior should be reserved for the case where a configured readout is absent from the loaded geometry).
if (m_cfg.readout.empty()) {
error("readoutClass is not provided, it is needed to know the fields in readout ids");
m_readout_available = false;
return;
}
Add eicrecon::geo::hasReadout / readoutIdSpec / readoutSegmentation in src/algorithms/interfaces/GeometryUtils.h. These query DD4hep readouts without throwing when a readout is absent (reporting absence via a bool or an empty std::optional), so algorithms can gracefully disable detector-specific code paths in reduced-geometry configurations instead of letting DD4hep's throwing Detector::readout() propagate through the JANA2 dataflow. A thrown exception continues to mean a genuine failure. The helpers are header-only and operate on a plain dd4hep::Detector, so they are usable from both the framework-independent algorithms (via algorithms::GeoSvc::instance().detector()) and from JANA services (via DD4hep_service::detector()). Adds a Catch2 unit test that exercises present and absent readouts against the mock detector and verifies the helpers do not throw where the raw DD4hep lookup does. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Implement Layer B by making geometry-dependent producers explicitly detect absent readouts at init-time and disable themselves instead of throwing through JANA's upstream producer path. For each affected algorithm, init now checks `eicrecon::geo::hasReadout(*detector, m_cfg.readout)` and, when absent, logs a warning and sets a disable flag. process() then returns early, emitting empty outputs. This keeps JANA semantics intact: missing collections remain optional-input behavior, while genuine algorithm failures still surface as exceptions. Applied to: - CalorimeterHitDigi - SimCalorimeterHitProcessor - CalorimeterIslandCluster - CalorimeterHitsMerger - MPGDTrackerDigi - MPGDHitReconstruction - LGADHitClustering - SiliconChargeSharing - PulseCombiner - FarDetectorTrackerCluster Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add a configurable `missingReadoutPolicy` knob (`disable` or `throw`) to the readout-dependent producer chain introduced in Layer B. - Added policy parsing utilities in GeometryUtils: - MissingReadoutPolicy enum - parseMissingReadoutPolicy() - Added `missingReadoutPolicy` to affected algorithm config structs with default `disable`. - Exposed `missingReadoutPolicy` as JANA parameters in corresponding factories. - Updated readout-gated algorithms so missing readouts now respect policy: - disable: warn once, disable producer, emit empty outputs - throw: raise runtime_error instead of disabling - Extended GeometryUtils unit tests for policy parsing. This keeps reduced-geometry default behavior stable while enabling strict mode for configurations that must fail on missing readouts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…d-geometry gating (fix: iwyu) (#2784) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/30136636279. Please merge this PR into the branch `nonthrowing-geometry-readout-api` to resolve failures in PR #2783. 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>
…d-geometry gating (fix: iwyu) (#2785) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/30137354767. Please merge this PR into the branch `nonthrowing-geometry-readout-api` to resolve failures in PR #2783. 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>
b3744d0 to
a37a113
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/factories/digi/PulseCombiner_factory.h:25
- The config/parameter key for
minimum_separationis spelledminimumSeperation(typo). This is user-facing (configurable via JANA parameters) and makes the knob harder to discover/remember. Consider correcting it tominimumSeparation(and optionally keeping the old spelling as a deprecated alias if backward compatibility is needed).
Briefly, what does this PR introduce? Please link to any relevant presentations or discussions.
This PR hardens geometry-dependent reconstruction in partial/reduced geometries while preserving strict failure behavior when desired.
It introduces:
src/algorithms/interfaces/GeometryUtils.h:hasReadout(...)readoutIdSpec(...)readoutSegmentation(...)parseMissingReadoutPolicy(...)withMissingReadoutPolicy::{Disable, Throw}missingReadoutPolicyknob (disabledefault,throwoptional) on readout-dependent producer configs and factory parameters.CalorimeterHitDigiSimCalorimeterHitProcessorCalorimeterIslandClusterCalorimeterHitsMergerMPGDTrackerDigiMPGDHitReconstructionLGADHitClusteringSiliconChargeSharingPulseCombinerFarDetectorTrackerClusterBehavior:
missingReadoutPolicy=disable(default): log a clear warning and emit empty outputs for that producer.missingReadoutPolicy=throw: raise a runtime error when the configured readout is absent.Motivation:
Validation:
cmake --build build --target install -- -j8ctest --test-dir build -V -R '^t_algorithms_test$'epic_craterlake_tracking_only, 2 events, CI gun flags):missingReadoutPolicy=throwpropagated missing-readout failures as expectedWhat is the urgency of this PR?
What kind of change does this PR introduce?
Please check if any of the following apply
Default-behavior change:
missingReadoutPolicy=disable) instead of propagating a readout lookup exception.AI usage: