From 2379c4c7746a49ba7cf7dee52ac45de847886139 Mon Sep 17 00:00:00 2001 From: Christopher Jones Date: Fri, 8 Jun 2018 16:40:09 -0500 Subject: [PATCH 1/2] Changed set to vector in PFDisplacedVertexSeed There was no fundamental reason to use a set rather than a vector. Even guaranteeing uniqueness of the elements in the container can be done efficiently with the vector. This should help with both memory size and speed. --- .../interface/PFDisplacedVertexSeed.h | 26 ++++--------- .../src/PFDisplacedVertexSeed.cc | 37 +++++++++++-------- .../PFTracking/src/PFDisplacedVertexFinder.cc | 11 ++---- 3 files changed, 34 insertions(+), 40 deletions(-) diff --git a/DataFormats/ParticleFlowReco/interface/PFDisplacedVertexSeed.h b/DataFormats/ParticleFlowReco/interface/PFDisplacedVertexSeed.h index da0ea925af0b9..305239c02737d 100644 --- a/DataFormats/ParticleFlowReco/interface/PFDisplacedVertexSeed.h +++ b/DataFormats/ParticleFlowReco/interface/PFDisplacedVertexSeed.h @@ -4,7 +4,7 @@ #include "DataFormats/GeometryVector/interface/GlobalPoint.h" #include "DataFormats/TrackReco/interface/TrackFwd.h" -#include +#include #include @@ -29,26 +29,16 @@ namespace reco { public: - /// -------- Useful Types -------- /// - - typedef std::set< reco::TrackBaseRef >::iterator IEset; - - /// A function necessary to use a set format to store the tracks Refs. - /// The set is the most appropriate in that case to avoid the double counting - /// of the tracks durring the build up procedure. - /// The position of the tracks in the Collection - /// is used as a classification parameter. - struct Compare{ - bool operator()(const TrackBaseRef& s1, const TrackBaseRef& s2) const - {return s1.key() < s2.key();} - }; - /// Default constructor PFDisplacedVertexSeed(); /// Add a track Reference to the current Seed + /// If the track reference is already in the collection, it is ignored void addElement(TrackBaseRef); + /// Reserve space for elements + void reserveElements(size_t); + /// Add a track Ref to the Seed and recalculate the seedPoint with a new dcaPoint /// A weight different from 1 may be assign to the new DCA point void updateSeedPoint(const GlobalPoint& dcaPoint, const TrackBaseRef, @@ -60,8 +50,8 @@ namespace reco { /// Check if it is a new Seed bool isEmpty() const {return (elements_.empty());} - /// \return set of references to tracks - const std::set < TrackBaseRef, Compare >& elements() const + /// \return vector of unique references to tracks + const std::vector & elements() const {return elements_;} const double nTracks() const {return elements_.size();} @@ -84,7 +74,7 @@ namespace reco { /// --------- MEMBERS ---------- /// /// Set of tracks refs associated to the seed - std::set < TrackBaseRef , Compare > elements_; + std::vector< TrackBaseRef> elements_; /// Seed point which indicated the approximative position of the vertex. GlobalPoint seedPoint_; /// Total weight of the points used to calculate the seed point. diff --git a/DataFormats/ParticleFlowReco/src/PFDisplacedVertexSeed.cc b/DataFormats/ParticleFlowReco/src/PFDisplacedVertexSeed.cc index 5985a5f5ead10..0029b9272cd9c 100644 --- a/DataFormats/ParticleFlowReco/src/PFDisplacedVertexSeed.cc +++ b/DataFormats/ParticleFlowReco/src/PFDisplacedVertexSeed.cc @@ -14,7 +14,13 @@ PFDisplacedVertexSeed::PFDisplacedVertexSeed() : void PFDisplacedVertexSeed::addElement(TrackBaseRef element) { - elements_.insert( element ); + if(std::find(elements_.begin(),elements_.end(), element) == elements_.end()) { + elements_.emplace_back(std::move(element)); + } +} + +void PFDisplacedVertexSeed::reserveElements(size_t newSize) { + elements_.reserve(newSize); } @@ -37,6 +43,7 @@ void PFDisplacedVertexSeed::updateSeedPoint(const GlobalPoint& dcaPoint, TrackBa } + reserveElements(elements_.size()+2); addElement(r1); addElement(r2); @@ -47,7 +54,6 @@ void PFDisplacedVertexSeed::mergeWith(const PFDisplacedVertexSeed& displacedVert double weight = displacedVertex.totalWeight(); - const set& newElements= displacedVertex.elements(); const GlobalPoint& dcaPoint = displacedVertex.seedPoint(); Basic3DVectorvertexSeedVector(seedPoint_); @@ -58,13 +64,14 @@ void PFDisplacedVertexSeed::mergeWith(const PFDisplacedVertexSeed& displacedVert totalWeight_ += weight; seedPoint_ = P; - - - for ( set::const_iterator il = newElements.begin(); il != newElements.end(); il++) - addElement(*il); - - - + reserveElements(elements_.size()+displacedVertex.elements().size()); + auto const oldSize=elements_.size(); + //avoid checking elements we just added from displacedVertex.elements() + for(auto const& e: displacedVertex.elements()) { + if(std::find(elements_.begin(), elements_.begin()+oldSize,e) == elements_.begin()+oldSize) { + elements_.emplace_back(e); + } + } } @@ -80,25 +87,25 @@ void PFDisplacedVertexSeed::Dump( ostream& out ) const { // Build element label (string) : elid from type, layer and occurence number // use stringstream instead of sprintf to concatenate string and integer into string - for(IEset ie = elements_.begin(); ie != elements_.end(); ie++){ + for(auto const& ie : elements_) { - math::XYZPoint Pi((*ie).get()->innerPosition()); - math::XYZPoint Po((*ie).get()->outerPosition()); + math::XYZPoint Pi(ie.get()->innerPosition()); + math::XYZPoint Po(ie.get()->outerPosition()); float innermost_radius = sqrt(Pi.x()*Pi.x() + Pi.y()*Pi.y() + Pi.z()*Pi.z()); float outermost_radius = sqrt(Po.x()*Po.x() + Po.y()*Po.y() + Po.z()*Po.z()); float innermost_rho = sqrt(Pi.x()*Pi.x() + Pi.y()*Pi.y()); float outermost_rho = sqrt(Po.x()*Po.x() + Po.y()*Po.y()); - double pt = (*ie)->pt(); + double pt = ie->pt(); - out<<"ie = " << (*ie).key() << " pt = " << pt + out<<"ie = " << ie.key() << " pt = " << pt <<" innermost hit radius = " << innermost_radius << " rho = " << innermost_rho <<" outermost hit radius = " << outermost_radius << " rho = " << outermost_rho < + + + + diff --git a/DataFormats/ParticleFlowReco/test/test_catch2_PFDisplacedVertexSeed.cc b/DataFormats/ParticleFlowReco/test/test_catch2_PFDisplacedVertexSeed.cc new file mode 100644 index 0000000000000..2bb4c97e646cb --- /dev/null +++ b/DataFormats/ParticleFlowReco/test/test_catch2_PFDisplacedVertexSeed.cc @@ -0,0 +1,104 @@ +#include "DataFormats/TrackReco/interface/Track.h" +#include "DataFormats/ParticleFlowReco/interface/PFDisplacedVertexSeed.h" + +#include "catch.hpp" + +static constexpr auto s_tag = "[PFDisplacedVertexSeed]"; +TEST_CASE("Check adding elements", s_tag) { + reco::PFDisplacedVertexSeed seed; + + REQUIRE(seed.elements().empty()); + + SECTION("updateSeedPoint") { + + //empty tracks are fine + std::vector tracks(5); + + seed.updateSeedPoint(GlobalPoint(0.01,0.01,0.01), + reco::TrackBaseRef(reco::TrackRef(&tracks,0)), + reco::TrackBaseRef(reco::TrackRef(&tracks,1)) ); + REQUIRE(seed.elements().size() == 2); + REQUIRE(seed.nTracks() == 2); + + seed.updateSeedPoint(GlobalPoint(0.01,0.01,0.01), + reco::TrackBaseRef(reco::TrackRef(&tracks,0)), + reco::TrackBaseRef(reco::TrackRef(&tracks,1)) ); + REQUIRE(seed.elements().size() == 2); + REQUIRE(seed.nTracks() == 2); + + seed.updateSeedPoint(GlobalPoint(0.01,0.01,0.01), + reco::TrackBaseRef(reco::TrackRef(&tracks,0)), + reco::TrackBaseRef(reco::TrackRef(&tracks,2)) ); + REQUIRE(seed.elements().size() == 3); + REQUIRE(seed.nTracks() == 3); + + seed.updateSeedPoint(GlobalPoint(0.01,0.01,0.01), + reco::TrackBaseRef(reco::TrackRef(&tracks,3)), + reco::TrackBaseRef(reco::TrackRef(&tracks,4)) ); + REQUIRE(seed.elements().size() == 5); + REQUIRE(seed.nTracks() == 5); + + } + + SECTION("addElement") { + //empty tracks are fine + std::vector tracks(3); + + seed.updateSeedPoint(GlobalPoint(0.01,0.01,0.01), + reco::TrackBaseRef(reco::TrackRef(&tracks,0)), + reco::TrackBaseRef(reco::TrackRef(&tracks,1)) ); + REQUIRE(seed.elements().size() == 2); + REQUIRE(seed.nTracks() == 2); + + seed.addElement(reco::TrackBaseRef(reco::TrackRef(&tracks,0))); + REQUIRE(seed.elements().size() == 2); + REQUIRE(seed.nTracks() == 2); + + seed.addElement(reco::TrackBaseRef(reco::TrackRef(&tracks,2))); + REQUIRE(seed.elements().size() == 3); + REQUIRE(seed.nTracks() == 3); + + } + SECTION("mergeWith") { + std::vector tracks(5); + + seed.updateSeedPoint(GlobalPoint(0.01,0.01,0.01), + reco::TrackBaseRef(reco::TrackRef(&tracks,0)), + reco::TrackBaseRef(reco::TrackRef(&tracks,1)) ); + + + SECTION("completely overlapping seeds") { + reco::PFDisplacedVertexSeed otherSeed; + otherSeed.updateSeedPoint(GlobalPoint(0.01,0.01,0.01), + reco::TrackBaseRef(reco::TrackRef(&tracks,0)), + reco::TrackBaseRef(reco::TrackRef(&tracks,1)) ); + + seed.mergeWith(otherSeed); + REQUIRE(seed.elements().size() == 2); + } + + SECTION("partially overlapping seeds") { + reco::PFDisplacedVertexSeed otherSeed; + otherSeed.updateSeedPoint(GlobalPoint(0.01,0.01,0.01), + reco::TrackBaseRef(reco::TrackRef(&tracks,0)), + reco::TrackBaseRef(reco::TrackRef(&tracks,2)) ); + + seed.mergeWith(otherSeed); + REQUIRE(seed.elements().size() == 3); + } + + SECTION("non overlapping seeds") { + REQUIRE(seed.elements().size()==2); + reco::PFDisplacedVertexSeed otherSeed; + otherSeed.updateSeedPoint(GlobalPoint(0.01,0.01,0.01), + reco::TrackBaseRef(reco::TrackRef(&tracks,3)), + reco::TrackBaseRef(reco::TrackRef(&tracks,2)) ); + REQUIRE(otherSeed.elements().size() == 2); + + seed.mergeWith(otherSeed); + REQUIRE(seed.elements().size() == 4); + } + + } +} + diff --git a/DataFormats/ParticleFlowReco/test/test_catch2_main.cc b/DataFormats/ParticleFlowReco/test/test_catch2_main.cc new file mode 100644 index 0000000000000..0c7c351f437f5 --- /dev/null +++ b/DataFormats/ParticleFlowReco/test/test_catch2_main.cc @@ -0,0 +1,2 @@ +#define CATCH_CONFIG_MAIN +#include "catch.hpp" diff --git a/RecoParticleFlow/PFTracking/test/BuildFile.xml b/RecoParticleFlow/PFTracking/test/BuildFile.xml index ce23400491db8..e40f6b92ab7eb 100644 --- a/RecoParticleFlow/PFTracking/test/BuildFile.xml +++ b/RecoParticleFlow/PFTracking/test/BuildFile.xml @@ -7,3 +7,7 @@ + + + + diff --git a/RecoParticleFlow/PFTracking/test/test_catch2_PFDisplacedVertexProducer.cc b/RecoParticleFlow/PFTracking/test/test_catch2_PFDisplacedVertexProducer.cc new file mode 100644 index 0000000000000..ea28a19618f9c --- /dev/null +++ b/RecoParticleFlow/PFTracking/test/test_catch2_PFDisplacedVertexProducer.cc @@ -0,0 +1,67 @@ +#include "catch.hpp" +#include "FWCore/TestProcessor/interface/TestProcessor.h" +#include "FWCore/Utilities/interface/Exception.h" +#include "DataFormats/ParticleFlowReco/interface/PFDisplacedVertexFwd.h" + + +static constexpr auto s_tag = "[PFDisplacedVertexProducer]"; + +TEST_CASE("Standard checks of PFDisplacedVertexProducer", s_tag) { + const std::string baseConfig{ +R"_(from FWCore.TestProcessor.TestProcess import * +process = TestProcess() +from RecoParticleFlow.PFTracking.particleFlowDisplacedVertex_cfi import particleFlowDisplacedVertex +process.toTest = particleFlowDisplacedVertex +process.moduleToTest(process.toTest) +)_" + }; + + const std::string fullConfig{ +R"_(from FWCore.TestProcessor.TestProcess import * +process = TestProcess() +process.load("MagneticField.Engine.uniformMagneticField_cfi") +process.load("Configuration.Geometry.GeometryExtended2018Reco_cff") +process.add_(cms.ESProducer("TrackerParametersESModule")) +process.load("Alignment.CommonAlignmentProducer.FakeAlignmentSource_cfi") + + +from RecoParticleFlow.PFTracking.particleFlowDisplacedVertex_cfi import particleFlowDisplacedVertex +process.toTest = particleFlowDisplacedVertex +process.moduleToTest(process.toTest) +)_" + }; + + edm::test::TestProcessor::Config config{ baseConfig }; + SECTION("base configuration is OK") { + REQUIRE_NOTHROW(edm::test::TestProcessor(config)); + } + + SECTION("No event data") { + edm::test::TestProcessor::Config config{ fullConfig }; + edm::test::TestProcessor tester(config); + + //The module ignores missing data products + REQUIRE(tester.test().get()->empty()); + } + + SECTION("beginJob and endJob only") { + edm::test::TestProcessor tester(config); + + REQUIRE_NOTHROW(tester.testBeginAndEndJobOnly()); + } + + SECTION("Run with no LuminosityBlocks") { + edm::test::TestProcessor tester(config); + + REQUIRE_NOTHROW(tester.testRunWithNoLuminosityBlocks()); + } + + SECTION("LuminosityBlock with no Events") { + edm::test::TestProcessor tester(config); + + REQUIRE_NOTHROW(tester.testLuminosityBlockWithNoEvents()); + } + +} + +//Add additional TEST_CASEs to exercise the modules capabilities diff --git a/RecoParticleFlow/PFTracking/test/test_catch2_main.cc b/RecoParticleFlow/PFTracking/test/test_catch2_main.cc new file mode 100644 index 0000000000000..0c7c351f437f5 --- /dev/null +++ b/RecoParticleFlow/PFTracking/test/test_catch2_main.cc @@ -0,0 +1,2 @@ +#define CATCH_CONFIG_MAIN +#include "catch.hpp"