Make sure to properly cleanup type names for patched subset collections

src/cpp/include/UTIL/CheckCollections.h

@@ -18,10 +18,31 @@ class PIDHandler;
  *  \date Dec 2022
  */
   class CheckCollections {
-
-    typedef std::vector<std::pair<std::string,std::string>> Vector ;
-
-  public: 
+
+  public:
+    /// Information about one collection
+    struct Collection {
+      Collection(std::string t, unsigned c, bool s)
+          : type(std::move(t)), count(c), subset(s) {}
+      Collection() = default;
+      Collection(const Collection &) = default;
+      Collection &operator=(const Collection &) = default;
+      Collection(Collection &&) = default;
+      Collection &operator=(Collection &&) = default;
+      ~Collection() = default;
+
+      std::string type{};
+      unsigned count{0};
+      bool subset{false};
+    };
+
+  private:
+
+    using CollectionVector = std::vector<std::pair<std::string, Collection>>;
+
+    using Vector = std::vector<std::pair<std::string, std::string>>;
+
+  public:
 
     /** Convenient c'tor.
      */
@@ -109,22 +130,6 @@ class PIDHandler;
       uint32_t count{};                      ///< How often this was found
     };
 
-    /// Information about one collection
-    struct Collection {
-      Collection(std::string t, unsigned c, bool s)
-        : type(std::move(t)), count(c), subset(s) {}
-      Collection() = default;
-      Collection(const Collection&) = default;
-      Collection& operator=(const Collection&) = default;
-      Collection(Collection&&) = default;
-      Collection& operator=(Collection&&) = default;
-      ~Collection() = default;
-
-      std::string type{};
-      unsigned count{0};
-      bool subset{false};
-    };
-
   private:
 
     void insertParticleIDMetas(const UTIL::PIDHandler& pidHandler, const std::string& recoName);
@@ -134,7 +139,7 @@ class PIDHandler;
     /// Map from ReconstructedParticle collection names to attached ParticleID
     /// meta information
     std::unordered_map<std::string, std::vector<PIDMeta>> _particleIDMetas{};
-    Vector _patchCols{};
+    CollectionVector _patchCols{};
 
   }; // class
 

src/cpp/src/UTIL/CheckCollections.cc

@@ -143,29 +143,35 @@ getRecoCollAndParamNames(const std::string_view fullType) {
   while (delim != std::string_view::npos) {
     auto oldDelim = delim + 1;
     delim = fullType.find(',', oldDelim);
-    paramNames.emplace_back(fullType.substr(oldDelim, delim));
+    paramNames.emplace_back(fullType.substr(oldDelim, delim - oldDelim));
   }
 
   return {recoName, paramNames};
 }
 
+// Check whether this collection should be patched as subset collection or not
+bool isSubsetCollection(const std::string_view fullType) {
+  return fullType.back() == '*';
+}
+
 void CheckCollections::addPatchCollection(std::string name, std::string type) {
   if (type.find('|') != std::string::npos) {
-    auto [recoName, paramNames] = getRecoCollAndParamNames(name);
+    auto [recoName, paramNames] = getRecoCollAndParamNames(type);
     _particleIDMetas[recoName].emplace_back(name, std::move(paramNames));
   } else {
-    _patchCols.emplace_back(std::move(name), std::move(type));
+    const auto isSubset = isSubsetCollection(type);
+    if (isSubset) {
+      type = type.substr(0, type.size() - 1);
+    }
+    _patchCols.emplace_back(std::piecewise_construct,
+                            std::forward_as_tuple(std::move(name)),
+                            std::forward_as_tuple(type, 0, isSubset));
   }
 }
 
 void CheckCollections::addPatchCollections(Vector cols) {
   for (auto &&[name, type] : cols) {
-    if (type.find('|') != std::string::npos) {
-      auto [recoName, paramNames] = getRecoCollAndParamNames(type);
-      _particleIDMetas[recoName].emplace_back(name, std::move(paramNames));
-    } else {
-      _patchCols.emplace_back(std::move(name), std::move(type));
-    }
+    addPatchCollection(std::move(name), std::move(type));
   }
 }
 
@@ -180,11 +186,6 @@ getToFromType(const std::string_view fullType) {
                                          2)}; // need to strip final "]" as well
 }
 
-// Check whether this collection should be patched as subset collection or not
-bool isSubsetCollection(const std::string_view fullType) {
-  return fullType.back() == '*';
-}
-
 // Add all algorithms that are specified in the pidMetas to the PIDHandler, such
 // that the necessary metadata is present
 void patchParticleIDs(UTIL::PIDHandler &pidHandler,
@@ -200,7 +201,8 @@ void patchParticleIDs(UTIL::PIDHandler &pidHandler,
 }
 
 void CheckCollections::patchCollections(EVENT::LCEvent *evt) const {
-  for (const auto &[name, typeName] : _patchCols) {
+  for (const auto &[name, collInfo] : _patchCols) {
+    const auto &typeName = collInfo.type;
     try {
       auto *coll = evt->getCollection(name);
       const auto collType = coll->getTypeName();
@@ -225,11 +227,11 @@ void CheckCollections::patchCollections(EVENT::LCEvent *evt) const {
         const auto [from, to] = getToFromType(typeName);
         params.setValue("FromType", std::string(from));
         params.setValue("ToType", std::string(to));
-        relationColl->setSubset(isSubsetCollection(typeName));
+        relationColl->setSubset(collInfo.subset);
         evt->addCollection(relationColl, name);
       } else {
         auto newColl = new IMPL::LCCollectionVec(typeName);
-        newColl->setSubset(isSubsetCollection(typeName));
+        newColl->setSubset(collInfo.subset);
         evt->addCollection(newColl, name);
       }
     }

tests/CMakeLists.txt

@@ -358,3 +358,4 @@ SET_TESTS_PROPERTIES( t_check_col_elements_maxevents2 PROPERTIES PASS_REGULAR_EX
 
 # ============================================================================
 
+add_subdirectory(unittests)

tests/unittests/CMakeLists.txt

@@ -0,0 +1,50 @@
+
+set(USE_EXTERNAL_CATCH2 AUTO CACHE STRING "Link against an external Catch2 v3 static library, otherwise build it locally")
+set_property(CACHE USE_EXTERNAL_CATCH2 PROPERTY STRINGS AUTO ON OFF)
+
+set(CATCH2_MIN_VERSION 3.5.0)
+if(USE_EXTERNAL_CATCH2)
+  if (USE_EXTERNAL_CATCH2 STREQUAL AUTO)
+    find_package(Catch2 ${CATCH2_MIN_VERSION})
+  else()
+    find_package(Catch2 ${CATCH2_MIN_VERSION} REQUIRED)
+  endif()
+endif()
+
+if(NOT Catch2_FOUND)
+  message(STATUS "Fetching local copy of Catch2 library for unit-tests...")
+  # Build Catch2 with the default flags, to avoid generating warnings when we
+  # build it
+  set(CXX_FLAGS_CMAKE_USED ${CMAKE_CXX_FLAGS})
+  set(CMAKE_CXX_FLAGS ${CXX_FLAGS_CMAKE_DEFAULTS})
+  Include(FetchContent)
+  FetchContent_Declare(
+    Catch2
+    GIT_REPOSITORY https://github.com/catchorg/Catch2.git
+    GIT_TAG        v${CATCH2_MIN_VERSION}
+    )
+  FetchContent_MakeAvailable(Catch2)
+  set(CMAKE_MODULE_PATH ${Catch2_SOURCE_DIR}/extras ${CMAKE_MODULE_PATH})
+
+  # Disable clang-tidy on external contents
+  set_target_properties(Catch2 PROPERTIES CXX_CLANG_TIDY "")
+
+  # Hack around the fact, that the include directories are not declared as
+  # SYSTEM for the targets defined this way. Otherwise warnings can still occur
+  # in Catch2 code when templates are evaluated (which happens quite a bit)
+  get_target_property(CATCH2_IF_INC_DIRS Catch2 INTERFACE_INCLUDE_DIRECTORIES)
+  set_target_properties(Catch2 PROPERTIES INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "${CATCH2_IF_INC_DIRS}")
+
+  # Reset the flags
+  set(CMAKE_CXX_FLAGS ${CXX_FLAGS_CMAKE_USED})
+endif()
+
+
+add_executable(unittests_lcio check_collections.cpp)
+target_link_libraries(unittests_lcio PRIVATE Catch2::Catch2WithMain LCIO::lcio)
+include(Catch)
+catch_discover_tests(unittests_lcio
+  TEST_PREFIX "ut_"
+  PROPERTIES
+    ENVIRONMENT LD_LIBRARY_PATH=$<TARGET_FILE_DIR:LCIO::lcio>:$ENV{LD_LIBRARY_PATH}
+)

tests/unittests/check_collections.cpp

@@ -0,0 +1,119 @@
+#include "IMPL/LCCollectionVec.h"
+#include "IMPL/LCEventImpl.h"
+#include "IMPL/MCParticleImpl.h"
+#include "IMPL/ReconstructedParticleImpl.h"
+#include "UTIL/CheckCollections.h"
+#include "UTIL/PIDHandler.h"
+
+#include <catch2/catch_test_macros.hpp>
+#include <catch2/matchers/catch_matchers_vector.hpp>
+
+TEST_CASE("CheckCollections patching basic collections", "[CheckCollections]") {
+  // Create an event that already contains a collection that we want to appear
+  // in the patched event
+  auto event = std::make_unique<IMPL::LCEventImpl>();
+  auto mcParticles =
+      std::make_unique<IMPL::LCCollectionVec>(EVENT::LCIO::MCPARTICLE);
+  auto mcp = new IMPL::MCParticleImpl();
+  mcParticles->addElement(mcp);
+  event->addCollection(mcParticles.release(), "TestMCParticles");
+
+  UTIL::CheckCollections checker;
+  checker.addPatchCollections({{"TestMCParticles", "MCParticle"},
+                               {"TestTracks", "Track"},
+                               {"TestSubsetCol", "CalorimeterHit*"}});
+  checker.patchCollections(event.get());
+
+  auto mcCol = event->getCollection("TestMCParticles");
+  REQUIRE(mcCol != nullptr);
+  REQUIRE(mcCol->getTypeName() == "MCParticle");
+  REQUIRE(mcCol->getNumberOfElements() == 1);
+  REQUIRE_FALSE(mcCol->isSubset());
+
+  auto trackCol = event->getCollection("TestTracks");
+  REQUIRE(trackCol != nullptr);
+  REQUIRE(trackCol->getTypeName() == "Track");
+  REQUIRE(trackCol->getNumberOfElements() == 0);
+  REQUIRE_FALSE(trackCol->isSubset());
+
+  auto subsetCol = event->getCollection("TestSubsetCol");
+  REQUIRE(subsetCol != nullptr);
+  REQUIRE(subsetCol->getTypeName() == "CalorimeterHit");
+  REQUIRE(subsetCol->getNumberOfElements() == 0);
+  REQUIRE(subsetCol->isSubset());
+}
+
+TEST_CASE("CheckCollections patching LCRelation collections",
+          "[CheckCollections]") {
+  auto event = std::make_unique<IMPL::LCEventImpl>();
+
+  UTIL::CheckCollections checker;
+  checker.addPatchCollection("MCTruthRelation",
+                             "LCRelation[MCParticle,ReconstructedParticle]");
+  checker.patchCollections(event.get());
+
+  auto relCol = event->getCollection("MCTruthRelation");
+  REQUIRE(relCol != nullptr);
+  REQUIRE(relCol->getTypeName() == "LCRelation");
+  REQUIRE(relCol->getNumberOfElements() == 0);
+
+  const auto &params = relCol->getParameters();
+  REQUIRE(params.getStringVal("FromType") == "MCParticle");
+  REQUIRE(params.getStringVal("ToType") == "ReconstructedParticle");
+}
+
+TEST_CASE("CheckCollections patching existing LCRelation without parameters",
+          "[CheckCollections]") {
+  // Create an event with the existing collection but missing From and To type
+  // parameters
+  auto event = std::make_unique<IMPL::LCEventImpl>();
+  auto existingRel = new IMPL::LCCollectionVec("LCRelation");
+  event->addCollection(existingRel, "ExistingRelation");
+
+  UTIL::CheckCollections checker;
+  checker.addPatchCollection("ExistingRelation", "LCRelation[Track,Cluster]");
+  checker.patchCollections(event.get());
+
+  auto relCol = event->getCollection("ExistingRelation");
+  const auto &params = relCol->getParameters();
+  REQUIRE(params.getStringVal("FromType") == "Track");
+  REQUIRE(params.getStringVal("ToType") == "Cluster");
+}
+
+TEST_CASE("CheckCollections patching ParticleID algorithms",
+          "[CheckCollections]") {
+  auto event = std::make_unique<IMPL::LCEventImpl>();
+
+  auto recoCol = new IMPL::LCCollectionVec("ReconstructedParticle");
+  auto recoPart = new IMPL::ReconstructedParticleImpl();
+  recoCol->addElement(recoPart);
+  event->addCollection(recoCol, "ReconstructedParticles");
+
+  UTIL::CheckCollections checker;
+  checker.addPatchCollections(
+      {{"LikelihoodPID", "ReconstructedParticles|dEdx,momentum"},
+       {"BDT_PID", "ReconstructedParticles|score"},
+       {"FancyPID", "ReconstructedParticles|param1,param2,param3"}});
+  checker.patchCollections(event.get());
+
+  auto pidHandler =
+      UTIL::PIDHandler(event->getCollection("ReconstructedParticles"));
+
+  int likelihoodID = -1;
+  int bdtID = -1;
+  int fancyID = -1;
+  REQUIRE_NOTHROW(likelihoodID = pidHandler.getAlgorithmID("LikelihoodPID"));
+  REQUIRE_NOTHROW(bdtID = pidHandler.getAlgorithmID("BDT_PID"));
+  REQUIRE_NOTHROW(fancyID = pidHandler.getAlgorithmID("FancyPID"));
+
+  const auto likelihoodParams = pidHandler.getParameterNames(likelihoodID);
+  REQUIRE_THAT(likelihoodParams,
+               Catch::Matchers::Equals<std::string>({"dEdx", "momentum"}));
+
+  const auto bdtParams = pidHandler.getParameterNames(bdtID);
+  REQUIRE_THAT(bdtParams, Catch::Matchers::Equals<std::string>({"score"}));
+
+  const auto fancyParams = pidHandler.getParameterNames(fancyID);
+  REQUIRE_THAT(fancyParams, Catch::Matchers::Equals<std::string>(
+                                {"param1", "param2", "param3"}));
+}