Skip to content

Reintegrated irt-2.1d changes into a master branch#2515

Open
alexander-kiselev wants to merge 125 commits into
mainfrom
irt-2.1e
Open

Reintegrated irt-2.1d changes into a master branch#2515
alexander-kiselev wants to merge 125 commits into
mainfrom
irt-2.1e

Conversation

@alexander-kiselev

@alexander-kiselev alexander-kiselev commented Feb 25, 2026

Copy link
Copy Markdown

Makes IRT2 codes consistent with irt-2.1e branches in EDM4eic/irt/epic repositories and 26.02.0-stable image

TODO:

  • Address remaining comments from Integrating IRT2 into EICRecon #2229
  • Pick up config files from geometry
  • Destructor of m_ReconstructionFactory crashes the reconstruction
  • Move logic from RICH IRT into algorithms init as much as possible
  • Get rid of G4DataInterpolation - the whole implementation is copied over and (incorrectly) used to intialize m_RefractiveIndex in IRT. Potential Geant4 licensing issues.
  • IrtConfig must be a POD type, it's value must not be mutated by the algorithm. All pointers need to move to the algorithm.
  • Adhoc tree/histogram outputs - this is generally not allowed. The functionality must be translated to PODIO macros. Alternatively we can setup a JEventProcessor that would consume m_EventBranch values, custom file output can be safely done there. TApplication/TCanvas must never be instantiated from an algorithm (this produces multiple instances).
    • validation benchmarks for IRT2 using PODIO outputs
    • (optional) calibrations scripts using PODIO outputs
  • License headers to reflect LGPL in spdx format

@github-actions github-actions Bot added topic: PID Relates to PID reconstruction topic: infrastructure labels Feb 25, 2026
alexander-kiselev and others added 26 commits March 9, 2026 08:15
```

/home/runner/work/EICrecon/EICrecon/src/algorithms/pid/G4DataInterpolation.cc: In member function 'double G4DataInterpolation::GetInterpolatedValue(double, order) const':
/home/runner/work/EICrecon/EICrecon/src/algorithms/pid/G4DataInterpolation.cc:85:14: error: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Werror=sign-compare]
   85 |     if (ibin == tdim - 1)
      |         ~~~~~^~~~~~~~~~~
cc1plus: all warnings being treated as errors
```
/home/runner/work/EICrecon/EICrecon/src/algorithms/pid/IrtInterfaceJsonParser.cc: In member function 'void eicrecon::IrtInterface::JsonParser()':
/home/runner/work/EICrecon/EICrecon/src/algorithms/pid/IrtInterfaceJsonParser.cc:104:26: error: comparison of integer expressions of different signedness: 'int' and 'std::map<TString, IRT2::CherenkovRadiator*>::size_type' {aka 'long unsigned int'} [-Werror=sign-compare]
  104 |                 if (rnum == reco->GetMyRICH()->Radiators().size()) {
      |                     ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/EICrecon/EICrecon/src/algorithms/pid/IrtInterfaceJsonParser.cc:110:44: error: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Werror=sign-compare]
  110 |                   for (unsigned ir = 0; ir < rnum; ir++)
      |                                         ~~~^~~~~~
This resolves conflict and brings this algorithm in compliance with the
interface.

In file included from /home/runner/work/EICrecon/EICrecon/src/algorithms/pid/IrtInterfaceJsonParser.cc:7:
/home/runner/work/EICrecon/EICrecon/src/algorithms/pid/IrtInterface.h:77:8: error: 'eicrecon::IrtInterface::init' hides overloaded virtual function [-Werror,-Woverloaded-virtual]
   77 |   void init(DD4hep_service& service, IrtConfig& config, std::shared_ptr<spdlog::logger>& logger);
      |        ^
/opt/local/include/algorithms/algorithm.h:87:16: note: hidden overloaded virtual function 'algorithms::Algorithm<algorithms::Input<edm4hep::MCParticleCollection, edm4eic::TrackCollection, edm4eic::MCRecoTrackParticleAssociationCollection, edm4eic::TrackSegmentCollection, edm4hep::SimTrackerHitCollection>, algorithms::Output<edm4eic::IrtRadiatorInfoCollection, edm4eic::IrtParticleCollection>>::init' declared here: different number of parameters (0 vs 3)
   87 |   virtual void init() {}
      |                ^
1 error generated.
In file included from /home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc:19:
/home/runner/work/EICrecon/EICrecon/src/extensions/jana/JOmniFactory.h:540:8: error: 'void JOmniFactory<AlgoT, ConfigT>::Process(const std::shared_ptr<const JEvent>&) [with AlgoT = eicrecon::IrtInterface_factory; ConfigT = eicrecon::IrtConfig]' was hidden [-Werror=overloaded-virtual=]
  540 |   void Process(const std::shared_ptr<const JEvent>& event) override {
      |        ^~~~~~~
In file included from /home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc:23:
/home/runner/work/EICrecon/EICrecon/src/global/pid/IrtInterface_factory.h:52:8: note:   by 'void eicrecon::IrtInterface_factory::Process(int64_t, uint64_t)'
   52 |   void Process(int64_t run_number, uint64_t event_number) {
      |        ^~~~~~~
/home/runner/work/EICrecon/EICrecon/src/extensions/jana/JOmniFactory.h:538:16: error: 'void JOmniFactory<AlgoT, ConfigT>::Process(int32_t, uint64_t) [with AlgoT = eicrecon::IrtInterface_factory; ConfigT = eicrecon::IrtConfig; int32_t = int; uint64_t = long unsigned int]' was hidden [-Werror=overloaded-virtual=]
  538 |   virtual void Process(int32_t /* run_number */, uint64_t /* event_number */) {};
      |                ^~~~~~~
/home/runner/work/EICrecon/EICrecon/src/global/pid/IrtInterface_factory.h:52:8: note:   by 'void eicrecon::IrtInterface_factory::Process(int64_t, uint64_t)'
   52 |   void Process(int64_t run_number, uint64_t event_number) {
      |        ^~~~~~~
/home/runner/work/EICrecon/EICrecon/src/extensions/jana/JOmniFactory.h:536:16: error: 'void JOmniFactory<AlgoT, ConfigT>::ChangeRun(int32_t) [with AlgoT = eicrecon::IrtInterface_factory; ConfigT = eicrecon::IrtConfig; int32_t = int]' was hidden [-Werror=overloaded-virtual=]
  536 |   virtual void ChangeRun(int32_t /* run_number */) override {};
      |                ^~~~~~~~~
/home/runner/work/EICrecon/EICrecon/src/global/pid/IrtInterface_factory.h:50:8: note:   by 'void eicrecon::IrtInterface_factory::ChangeRun(int64_t)'
   50 |   void ChangeRun(int64_t run_number) {}
      |        ^~~~~~~~~
/home/runner/work/EICrecon/EICrecon/src/global/pid/IrtInterface_factory.h: In member function 'void eicrecon::IrtInterface_factory::ChangeRun(int64_t)':
/home/runner/work/EICrecon/EICrecon/src/global/pid/IrtInterface_factory.h:50:26: error: unused parameter 'run_number' [-Werror=unused-parameter]
   50 |   void ChangeRun(int64_t run_number) {}
      |                  ~~~~~~~~^~~~~~~~~~
/home/runner/work/EICrecon/EICrecon/src/global/pid/IrtInterface_factory.h: In member function 'void eicrecon::IrtInterface_factory::Process(int64_t, uint64_t)':
/home/runner/work/EICrecon/EICrecon/src/global/pid/IrtInterface_factory.h:52:24: error: unused parameter 'run_number' [-Werror=unused-parameter]
   52 |   void Process(int64_t run_number, uint64_t event_number) {
      |                ~~~~~~~~^~~~~~~~~~
/home/runner/work/EICrecon/EICrecon/src/global/pid/IrtInterface_factory.h:52:45: error: unused parameter 'event_number' [-Werror=unused-parameter]
   52 |   void Process(int64_t run_number, uint64_t event_number) {
      |                                    ~~~~~~~~~^~~~~~~~~~~~
/home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc: In function 'void InitPlugin(JApplication*)':
/home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc:171:20: error: unused variable 'bf' [-Werror=unused-variable]
  171 |               bool bf = sf->GetCrossing(x0, n0, &from, false);
      |                    ^~
/home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc:172:20: error: unused variable 'br' [-Werror=unused-variable]
  172 |               bool br = sr->GetCrossing(x0, n0, &to, false);
      |                    ^~
/home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc:186:31: error: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Werror=sign-compare]
  186 |             for (int i = 0; i < numPlanes; i++) {
      |                             ~~^~~~~~~~~~~
/home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc: In lambda function:
/home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc:213:79: error: unused parameter 'p' [-Werror=unused-parameter]
  213 |               std::function<bool(edm4eic::TrackPoint)>([](edm4eic::TrackPoint p) { return true; });
      |                                                           ~~~~~~~~~~~~~~~~~~~~^
/home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc: In function 'void InitPlugin(JApplication*)':
/home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc:338:22: error: unused variable 'bf' [-Werror=unused-variable]
  338 |                 bool bf = sf->GetCrossing(x0, n0, &from, false);
      |                      ^~
/home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc:339:22: error: unused variable 'br' [-Werror=unused-variable]
  339 |                 bool br = sr->GetCrossing(x0, n0, &to, false);
      |                      ^~
/home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc:353:33: error: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Werror=sign-compare]
  353 |               for (int i = 0; i < numPlanes; i++) {
      |                               ~~^~~~~~~~~~~
/home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc: In lambda function:
/home/runner/work/EICrecon/EICrecon/src/detectors/RICH-IRT/RICH-IRT.cc:380:40: error: unused parameter 'p' [-Werror=unused-parameter]
  380 |                 [](edm4eic::TrackPoint p) { return true; });
      |                    ~~~~~~~~~~~~~~~~~~~~^
cc1plus: all warnings being treated as errors
Comment thread src/algorithms/pid/IrtInterface.cc
Comment thread src/algorithms/pid/IrtInterface.cc
Comment thread src/algorithms/pid/IrtInterface.cc
Comment thread src/algorithms/pid/IrtInterface.cc Outdated
Comment thread src/algorithms/pid/IrtInterface.cc Outdated
Comment on lines +52 to +69
if (m_ReconstructionFactory) {
delete m_ReconstructionFactory;
m_ReconstructionFactory = nullptr;
} //if

if (m_Event) {
delete m_Event;
m_Event = nullptr;
} //if

if (m_irt_detector) {
for (auto [name, rad] : m_irt_detector->Radiators()) {
if (rad && rad->m_RefractiveIndex) {
delete rad->m_RefractiveIndex;
rad->m_RefractiveIndex = nullptr;
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use smart pointers here and rely on RAII for deletion? Likely makes destructor sufficient as default.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought too. I suspect IRT2 won't use smart pointers inside due to ROOT streamer issues. I will treat m_Event/m_ReconstructionFactory that we have full control over.

Comment thread src/global/pid/IrtInterface_factory.h Outdated
Comment thread src/utilities/eicrecon/eicrecon.cc
Comment thread src/utilities/eicrecon/CMakeLists.txt
Comment on lines +67 to +68
auto dd4hep_service = app->GetService<DD4hep_service>();
auto det = dd4hep_service->detector();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anti-pattern to parse geometry in plugin. In this case we don't use the geometry beyond checking that the specific RICH is there, so we can skip this here and rely on the algorithm not doing anything if the RICH is not in the geometry.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I think this is a valid question and the FIXME states as much. I suspect, there might be a crash whenever IRT2 tries to access a detector that is not in the configuration, and this is a guard against that. I don't think it's in scope to fix that now - we'll need to revisit this.

Comment thread src/detectors/RICH-IRT/RICH-IRT.cc Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: infrastructure topic: PID Relates to PID reconstruction

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants