From 625de18b4e0e0ab562368f92e83ad2147dd766e4 Mon Sep 17 00:00:00 2001
From: Markus Frank <Markus.Frank@cern.ch>
Date: Mon, 16 Jan 2023 20:25:15 +0100
Subject: [PATCH] DDDigi cleanup input modules

---
 DDDigi/include/DDDigi/DigiROOTInput.h       |  2 +-
 DDDigi/io/DigiDDG4Input.cpp                 | 18 +++---
 DDDigi/io/DigiEdm4hepInput.cpp              | 37 +++++++-----
 DDDigi/src/DigiROOTInput.cpp                | 65 ++++++++-------------
 examples/DDDigi/scripts/TestEdm4hepInput.py |  2 +-
 5 files changed, 60 insertions(+), 64 deletions(-)

diff --git a/DDDigi/include/DDDigi/DigiROOTInput.h b/DDDigi/include/DDDigi/DigiROOTInput.h
index f2612156d..a2e7ed457 100644
--- a/DDDigi/include/DDDigi/DigiROOTInput.h
+++ b/DDDigi/include/DDDigi/DigiROOTInput.h
@@ -32,7 +32,7 @@ namespace dd4hep {
     // Forward declarations
     class DigiROOTInput;
 
-    /// Base class for input actions to the digitization
+    /// Base class for input actions to the digitization using ROOT
     /**
      *
      *  \author  M.Frank
diff --git a/DDDigi/io/DigiDDG4Input.cpp b/DDDigi/io/DigiDDG4Input.cpp
index 88a1e1ae3..00572d684 100644
--- a/DDDigi/io/DigiDDG4Input.cpp
+++ b/DDDigi/io/DigiDDG4Input.cpp
@@ -11,27 +11,31 @@
 //
 //==========================================================================
 
-// Framework include files
+/// Framework include files
 #include <DDDigi/DigiROOTInput.h>
 #include "DigiIO.h"
 
 #include <DDG4/Geant4Data.h>
 #include <DDG4/Geant4Particle.h>
 
-// ROOT include files
-#include <TBranch.h>
-#include <TClass.h>
+/// ROOT include files
 #include <TROOT.h>
+#include <TBranch.h>
 
 /// Namespace for the AIDA detector description toolkit
 namespace dd4hep {
 
   /// Namespace for the Digitization part of the AIDA detector description toolkit
   namespace digi {
-    
-    using namespace std::placeholders;
 
-    class DigiDDG4ROOT : public DigiROOTInput    {
+    /// DDDigi input reader for DDG4 native ROOT output
+    /**
+     *
+     *  \author  M.Frank
+     *  \version 1.0
+     *  \ingroup DD4HEP_DIGITIZATION
+     */
+   class DigiDDG4ROOT : public DigiROOTInput    {
     public:
       /// Class pointers of the objects to be imported
       TClass* m_trackerHitClass { nullptr };
diff --git a/DDDigi/io/DigiEdm4hepInput.cpp b/DDDigi/io/DigiEdm4hepInput.cpp
index ee06258c6..9415f19d3 100644
--- a/DDDigi/io/DigiEdm4hepInput.cpp
+++ b/DDDigi/io/DigiEdm4hepInput.cpp
@@ -16,8 +16,6 @@
 #include <DDDigi/DigiData.h>
 #include "DigiIO.h"
 
-// C/C++ include files
-
 /// Forward declarations
 namespace podio {  class CollectionBase; }
 
@@ -40,9 +38,9 @@ namespace dd4hep {
 
       /// Forward declarations
       class internals_t;
-      class inputsource_t;
-      class collection_t;
       class work_t;
+      class source_t;
+      class collection_t;
       using podio_coll_t = const podio::CollectionBase;
       using descriptor_t = std::pair<const Key, collection_t>;
 
@@ -59,6 +57,7 @@ namespace dd4hep {
       /// Initializing constructor
       DigiEdm4hepInput(const DigiKernel& krnl, const std::string& nam);
 
+      /// Generic conversion function for hits
       template <typename T, typename COLL>
       void from_edm4hep(DigiContext&       context,
 			DataSegment&       segment,
@@ -81,6 +80,7 @@ namespace dd4hep {
 	}
       }
 
+      /// Generic conversion function for MC particles
       template <typename COLL>
       void from_edm4hep(DigiContext&       context,
 			DataSegment&       segment,
@@ -118,6 +118,13 @@ namespace dd4hep {
   /// Namespace for the Digitization part of the AIDA detector description toolkit
   namespace digi {
 
+    /// EDM4HEP event frame wrapper
+    /**
+     *
+     *  \author  M.Frank
+     *  \version 1.0
+     *  \ingroup DD4HEP_DIGITIZATION
+     */
     class edm4hep_read_frame_t : public DigiInputAction::event_frame   {
     public:
       podio::Frame frame { };
@@ -162,7 +169,7 @@ namespace dd4hep {
      *  \version 1.0
      *  \ingroup DD4HEP_DIGITIZATION
      */
-    class DigiEdm4hepInput::inputsource_t : public DigiInputAction::input_source   {
+    class DigiEdm4hepInput::source_t : public DigiInputAction::input_source   {
     public:
       /// Reference to the reader object
       std::unique_ptr<reader_t>   stream      { };
@@ -175,11 +182,11 @@ namespace dd4hep {
 
     public:
       /// Initializing constructor
-      inputsource_t(const std::string& s, std::unique_ptr<reader_t>&& str) 
+      source_t(const std::string& s, std::unique_ptr<reader_t>&& str) 
 	: stream(std::move(str)), section(s)  {
       }
       /// Default destructor
-      ~inputsource_t()   {
+      ~source_t()   {
 	if ( stream )    {
 	  stream.reset();
 	}
@@ -212,7 +219,7 @@ namespace dd4hep {
      */
     class DigiEdm4hepInput::internals_t  {
     public:
-      using input_t = std::unique_ptr<inputsource_t>;
+      using input_t = std::unique_ptr<source_t>;
 
       /// Reference to the parent object
       DigiEdm4hepInput* m_parent { nullptr };
@@ -224,8 +231,8 @@ namespace dd4hep {
     public:
       /// Initializing constructor
       internals_t(DigiEdm4hepInput* parent);
-      /// Open new input stream
-      std::unique_ptr<inputsource_t> open_next_data_source();
+      /// Open the next input source from the input list
+      std::unique_ptr<source_t> open_source();
       /// Access the next event from the sequence of input files
       std::shared_ptr<frame_t> next();
     };
@@ -236,8 +243,9 @@ namespace dd4hep {
     {
     }
 
-    std::unique_ptr<DigiEdm4hepInput::inputsource_t>
-    DigiEdm4hepInput::internals_t::open_next_data_source()   {
+    /// Open the next input source from the input list
+    std::unique_ptr<DigiEdm4hepInput::source_t>
+    DigiEdm4hepInput::internals_t::open_source()   {
       const auto& inputs = m_parent->inputs();
       int len = inputs.size();
       if ( inputs.empty() ) m_curr_input = 0;
@@ -247,7 +255,7 @@ namespace dd4hep {
 	try  {
 	  auto sec = m_parent->input_section();
 	  stream->openFile(fname);
-	  auto source = std::make_unique<inputsource_t>(sec, std::move(stream));
+	  auto source = std::make_unique<source_t>(sec, std::move(stream));
 	  m_parent->info("+++ Opened EDM4HEP input file %s.", fname.c_str());
 	  m_parent->onOpenFile(*source);
 	  return source;
@@ -260,10 +268,11 @@ namespace dd4hep {
       throw std::runtime_error("+++ No open file present");
     }
 
+    /// Access the next event record. If the courrent source is exhausted, open next source
     std::shared_ptr<frame_t> DigiEdm4hepInput::internals_t::next()   {
       if ( !m_source || m_source->done() || m_parent->fileLimitReached(*m_source) )    {
 	int mask = m_parent->input_mask();
-	m_source = open_next_data_source();
+	m_source = open_source();
 	if ( m_source )   {
 	  auto frame = m_source->next();
 	  if ( frame )   {
diff --git a/DDDigi/src/DigiROOTInput.cpp b/DDDigi/src/DigiROOTInput.cpp
index b7bc81b8d..a273441db 100644
--- a/DDDigi/src/DigiROOTInput.cpp
+++ b/DDDigi/src/DigiROOTInput.cpp
@@ -14,46 +14,37 @@
 // Framework include files
 #include <DD4hep/InstanceCount.h>
 #include <DDDigi/DigiROOTInput.h>
-#include <DD4hep/Printout.h>
-#include <DD4hep/Factories.h>
 
 // ROOT include files
 #include <TROOT.h>
 #include <TFile.h>
 #include <TTree.h>
 
-// C/C++ include files
-#include <stdexcept>
-#include <map>
-
 using namespace dd4hep::digi;
 
 class DigiROOTInput::inputsource_t
-  : public DigiInputAction::input_source,
-    public DigiInputAction::event_frame
-{
+  : public DigiInputAction::input_source, public DigiInputAction::event_frame  {
 public:
   /// Branches present in the current file
-  std::map<Key, container_t>  branches  { };
+  std::map<Key, container_t> branches { };
   /// Reference to the current ROOT file to be read
-  TFile*   file   { nullptr };
+  std::unique_ptr<TFile>     file     { nullptr };
   /// Reference to the ROOT tree to be read
   TTree*   tree   { nullptr };
   /// Current entry inside the current input source
   Long64_t entry  { -1 };
 
 public:
+  /// Default constructor
   inputsource_t() = default;
-  ~inputsource_t()   {
-    if ( file )    {
-      file->Close();
-      delete file;
-      file = nullptr;
-    }
+  /// Default destructor
+  ~inputsource_t() = default;
+  /// Check if the input source is exhausted
+  bool done()   const     {
+    return (entry+1) >= tree->GetEntries();
   }
   inputsource_t& next()   {
-    ++entry;
-    tree->LoadTree( entry );
+    tree->LoadTree( ++entry );
     return *this;
   }
 };
@@ -67,11 +58,11 @@ public:
  */
 class DigiROOTInput::internals_t   {
 public:
-  using handle_t = std::unique_ptr<inputsource_t>;
+  using source_t = std::unique_ptr<inputsource_t>;
   /// Reference to parent action
   DigiROOTInput* m_parent       { nullptr };
   /// Handle to input source
-  handle_t       m_source       { };
+  source_t       m_source       { };
   /// Pointer to current input source
   int            m_curr_input   { INPUT_START };
 
@@ -82,8 +73,8 @@ public:
   ~internals_t () = default;
   /// Access the next valid event entry
   inputsource_t& next();
-  /// Open the next input source
-  std::unique_ptr<inputsource_t> open_next_data_source();
+  /// Open the next input source from the input list
+  std::unique_ptr<inputsource_t> open_source();
 };
 
 /// Default constructor
@@ -92,8 +83,8 @@ DigiROOTInput::internals_t::internals_t (DigiROOTInput* p)
 {
 }
 
-std::unique_ptr<DigiROOTInput::inputsource_t>
-DigiROOTInput::internals_t::open_next_data_source()   {
+/// Open the next input source from the input list
+std::unique_ptr<DigiROOTInput::inputsource_t> DigiROOTInput::internals_t::open_source()   {
   const auto& inputs    = m_parent->inputs();
   const auto& tree_name = m_parent->input_section();
   int len = inputs.size();
@@ -101,10 +92,9 @@ DigiROOTInput::internals_t::open_next_data_source()   {
   if ( inputs.empty() ) m_curr_input = 0;
   while ( (m_curr_input+1) < len )   {
     const auto& fname = inputs[++m_curr_input];
-    auto* file = TFile::Open(fname.c_str());
+    std::unique_ptr<TFile> file(TFile::Open(fname.c_str()));
     if ( file && file->IsZombie() )  {
-      delete file;
-      file = nullptr;
+      file.reset();
       m_parent->error("OpenInput ++ Failed to open input source %s", fname.c_str());
     }
     else if ( file )  {
@@ -120,12 +110,9 @@ DigiROOTInput::internals_t::open_next_data_source()   {
 			tree_name.c_str(), fname.c_str());
 	continue;
       }
-      auto source = std::make_unique<inputsource_t>();
-      source->branches.clear();
-      source->file  = file;
+      auto source   = std::make_unique<inputsource_t>();
+      source->file  = std::move(file);
       source->tree  = tree;
-      source->entry = -1;
-
       auto* branches = tree->GetListOfBranches();
       int mask = m_parent->input_mask();
       TObjArrayIter it(branches);
@@ -145,17 +132,13 @@ DigiROOTInput::internals_t::open_next_data_source()   {
     }
   }
   m_parent->except("+++ No open file present. Configuration error?");
-  throw std::runtime_error("+++ No open file present");
+  return {};
 }
 
+/// Access the next event from the sequence of input files
 DigiROOTInput::inputsource_t& DigiROOTInput::internals_t::next()   {
-  if ( !m_source || m_parent->fileLimitReached(*m_source) )    {
-    m_source = open_next_data_source();
-  }
-  Int_t total = m_source->tree->GetEntries();
-  if ( (1+m_source->entry) >= total )   {
-    m_source.reset();
-    m_source = open_next_data_source();
+  if ( !m_source || m_source->done() || m_parent->fileLimitReached(*m_source) )    {
+    m_source = open_source();
   }
   auto& src = m_source->next();
   m_parent->onProcessEvent(src, src);
diff --git a/examples/DDDigi/scripts/TestEdm4hepInput.py b/examples/DDDigi/scripts/TestEdm4hepInput.py
index ffc11184e..aeffb896d 100644
--- a/examples/DDDigi/scripts/TestEdm4hepInput.py
+++ b/examples/DDDigi/scripts/TestEdm4hepInput.py
@@ -18,7 +18,7 @@ def run():
                            mask=0x0,
                            input=['MiniTel_DDG4_edm4hep_data.run00000000.root',
                                   'MiniTel_DDG4_edm4hep_data.run00000001.root',
-                                  'MiniTel_DDG4_edm4hep_data.run00000002.root',])
+                                  'MiniTel_DDG4_edm4hep_data.run00000002.root'])
   read.input_section = 'events'
   read.objects_disabled = ['EventHeader']
   read.events_per_file = 5
-- 
GitLab