From cf0b56c1565181bc38bd90276ccacda2ea0e966f Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Mon, 8 Sep 2025 13:30:21 +0200 Subject: [PATCH 1/5] Fix bug in getting particle ID meta information --- src/cpp/src/UTIL/CheckCollections.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpp/src/UTIL/CheckCollections.cc b/src/cpp/src/UTIL/CheckCollections.cc index 80c44cac3..c92d9672d 100644 --- a/src/cpp/src/UTIL/CheckCollections.cc +++ b/src/cpp/src/UTIL/CheckCollections.cc @@ -151,7 +151,7 @@ getRecoCollAndParamNames(const std::string_view fullType) { 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)); From 4da7ba338bf8ca43c1df6823b3995abe2c112ec6 Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Mon, 8 Sep 2025 13:30:51 +0200 Subject: [PATCH 2/5] Make sure to strip '*' from patched subset collection types LCIO doesn't really care about this, but the typename is potentially used downstream --- src/cpp/include/UTIL/CheckCollections.h | 47 ++++++++++++++----------- src/cpp/src/UTIL/CheckCollections.cc | 32 +++++++++-------- 2 files changed, 43 insertions(+), 36 deletions(-) diff --git a/src/cpp/include/UTIL/CheckCollections.h b/src/cpp/include/UTIL/CheckCollections.h index c19e01314..e7ae22d73 100644 --- a/src/cpp/include/UTIL/CheckCollections.h +++ b/src/cpp/include/UTIL/CheckCollections.h @@ -18,10 +18,31 @@ class PIDHandler; * \date Dec 2022 */ class CheckCollections { - - typedef std::vector> 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>; + + using Vector = std::vector>; + + 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> _particleIDMetas{}; - Vector _patchCols{}; + CollectionVector _patchCols{}; }; // class diff --git a/src/cpp/src/UTIL/CheckCollections.cc b/src/cpp/src/UTIL/CheckCollections.cc index c92d9672d..ce0b58eaf 100644 --- a/src/cpp/src/UTIL/CheckCollections.cc +++ b/src/cpp/src/UTIL/CheckCollections.cc @@ -149,23 +149,29 @@ getRecoCollAndParamNames(const std::string_view fullType) { 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(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); } } From 295996c797f79eb95805a919fc8eb55200150984 Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Mon, 8 Sep 2025 15:07:23 +0200 Subject: [PATCH 3/5] Add unittests for patching collections --- tests/CMakeLists.txt | 1 + tests/unittests/CMakeLists.txt | 50 +++++++++++ tests/unittests/check_collections.cpp | 117 ++++++++++++++++++++++++++ 3 files changed, 168 insertions(+) create mode 100644 tests/unittests/CMakeLists.txt create mode 100644 tests/unittests/check_collections.cpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ceed191d3..c6b1c2a95 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -358,3 +358,4 @@ SET_TESTS_PROPERTIES( t_check_col_elements_maxevents2 PROPERTIES PASS_REGULAR_EX # ============================================================================ +add_subdirectory(unittests) diff --git a/tests/unittests/CMakeLists.txt b/tests/unittests/CMakeLists.txt new file mode 100644 index 000000000..32d44426a --- /dev/null +++ b/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=$:$ENV{LD_LIBRARY_PATH} +) diff --git a/tests/unittests/check_collections.cpp b/tests/unittests/check_collections.cpp new file mode 100644 index 000000000..13446794a --- /dev/null +++ b/tests/unittests/check_collections.cpp @@ -0,0 +1,117 @@ +#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 +#include + +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(); + event->setRunNumber(1); + event->setEventNumber(1); + auto mcParticles = + std::make_unique(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(); + + 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 ¶ms = 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(); + 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 ¶ms = relCol->getParameters(); + REQUIRE(params.getStringVal("FromType") == "Track"); + REQUIRE(params.getStringVal("ToType") == "Cluster"); +} + +TEST_CASE("CheckCollections patching ParticleID algorithms", + "[CheckCollections]") { + auto event = std::make_unique(); + event->setRunNumber(1); + event->setEventNumber(1); + + 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,confidence"}}); + checker.patchCollections(event.get()); + + auto pidHandler = + UTIL::PIDHandler(event->getCollection("ReconstructedParticles")); + + int likelihoodID = -1; + int bdtID = -1; + REQUIRE_NOTHROW(likelihoodID = pidHandler.getAlgorithmID("LikelihoodPID")); + REQUIRE_NOTHROW(bdtID = pidHandler.getAlgorithmID("BDT_PID")); + + auto likelihoodParams = pidHandler.getParameterNames(likelihoodID); + REQUIRE_THAT(likelihoodParams, Catch::Matchers::UnorderedEquals( + {"dEdx", "momentum"})); + + auto bdtParams = pidHandler.getParameterNames(bdtID); + REQUIRE_THAT(bdtParams, Catch::Matchers::UnorderedEquals( + {"score", "confidence"})); +} From 554253004c99187397114153136d73c04f4d6fc9 Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Mon, 8 Sep 2025 16:17:45 +0200 Subject: [PATCH 4/5] Fix bug in parsing of parameter names --- src/cpp/src/UTIL/CheckCollections.cc | 2 +- tests/unittests/check_collections.cpp | 20 +++++++++++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/cpp/src/UTIL/CheckCollections.cc b/src/cpp/src/UTIL/CheckCollections.cc index ce0b58eaf..fa605fd31 100644 --- a/src/cpp/src/UTIL/CheckCollections.cc +++ b/src/cpp/src/UTIL/CheckCollections.cc @@ -143,7 +143,7 @@ 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}; diff --git a/tests/unittests/check_collections.cpp b/tests/unittests/check_collections.cpp index 13446794a..2281180d0 100644 --- a/tests/unittests/check_collections.cpp +++ b/tests/unittests/check_collections.cpp @@ -96,7 +96,8 @@ TEST_CASE("CheckCollections patching ParticleID algorithms", UTIL::CheckCollections checker; checker.addPatchCollections( {{"LikelihoodPID", "ReconstructedParticles|dEdx,momentum"}, - {"BDT_PID", "ReconstructedParticles|score,confidence"}}); + {"BDT_PID", "ReconstructedParticles|score"}, + {"FancyPID", "ReconstructedParticles|param1,param2,param3"}}); checker.patchCollections(event.get()); auto pidHandler = @@ -104,14 +105,19 @@ TEST_CASE("CheckCollections patching ParticleID algorithms", 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")); - auto likelihoodParams = pidHandler.getParameterNames(likelihoodID); - REQUIRE_THAT(likelihoodParams, Catch::Matchers::UnorderedEquals( - {"dEdx", "momentum"})); + const auto likelihoodParams = pidHandler.getParameterNames(likelihoodID); + REQUIRE_THAT(likelihoodParams, + Catch::Matchers::Equals({"dEdx", "momentum"})); - auto bdtParams = pidHandler.getParameterNames(bdtID); - REQUIRE_THAT(bdtParams, Catch::Matchers::UnorderedEquals( - {"score", "confidence"})); + const auto bdtParams = pidHandler.getParameterNames(bdtID); + REQUIRE_THAT(bdtParams, Catch::Matchers::Equals({"score"})); + + const auto fancyParams = pidHandler.getParameterNames(fancyID); + REQUIRE_THAT(fancyParams, Catch::Matchers::Equals( + {"param1", "param2", "param3"})); } From 31e04e302d5f81b71b33514f0b71e5ca60e7d3db Mon Sep 17 00:00:00 2001 From: Thomas Madlener Date: Tue, 9 Sep 2025 10:18:43 +0200 Subject: [PATCH 5/5] Remove unnecessary setting of event and run number in tests --- tests/unittests/check_collections.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/unittests/check_collections.cpp b/tests/unittests/check_collections.cpp index 2281180d0..0f3a4fc44 100644 --- a/tests/unittests/check_collections.cpp +++ b/tests/unittests/check_collections.cpp @@ -12,8 +12,6 @@ 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(); - event->setRunNumber(1); - event->setEventNumber(1); auto mcParticles = std::make_unique(EVENT::LCIO::MCPARTICLE); auto mcp = new IMPL::MCParticleImpl(); @@ -85,8 +83,6 @@ TEST_CASE("CheckCollections patching existing LCRelation without parameters", TEST_CASE("CheckCollections patching ParticleID algorithms", "[CheckCollections]") { auto event = std::make_unique(); - event->setRunNumber(1); - event->setEventNumber(1); auto recoCol = new IMPL::LCCollectionVec("ReconstructedParticle"); auto recoPart = new IMPL::ReconstructedParticleImpl();