Merge branch 'ATLASSIM-3365-ISF-improve-CollectionMerger-robustness' into '21.0'

Make ISF CollectionMerger correctly copy pointer-type hits for `AthenaHitsVector` collections

See merge request !3181

Former-commit-id: e77320f7

Simulation/ISF/ISF_Core/ISF_Algorithms/src/CollectionMerger.cxx

@@ -153,24 +153,24 @@ StatusCode ISF::CollectionMerger::initialize()
 /** Athena Algorithm execute */
 StatusCode ISF::CollectionMerger::execute()
 {
-  mergeCollections( m_inputBCMHits,             m_outputBCMHits             );
-  mergeCollections( m_inputBLMHits,             m_outputBLMHits             );
-  mergeCollections( m_inputPixelHits,           m_outputPixelHits           );
-  mergeCollections( m_inputSCTHits,             m_outputSCTHits             );
-  mergeCollections( m_inputTRTUncompressedHits, m_outputTRTUncompressedHits );
+  ATH_CHECK(mergeCollections( m_inputBCMHits,             m_outputBCMHits             ));
+  ATH_CHECK(mergeCollections( m_inputBLMHits,             m_outputBLMHits             ));
+  ATH_CHECK(mergeCollections( m_inputPixelHits,           m_outputPixelHits           ));
+  ATH_CHECK(mergeCollections( m_inputSCTHits,             m_outputSCTHits             ));
+  ATH_CHECK(mergeCollections( m_inputTRTUncompressedHits, m_outputTRTUncompressedHits ));
 
-  mergeCollections( m_inputLArEMBHits,          m_outputLArEMBHits          );
-  mergeCollections( m_inputLArEMECHits,         m_outputLArEMECHits         );
-  mergeCollections( m_inputLArFCALHits,         m_outputLArFCALHits         );
-  mergeCollections( m_inputLArHECHits,          m_outputLArHECHits          );
+  ATH_CHECK(mergeCollections( m_inputLArEMBHits,          m_outputLArEMBHits          ));
+  ATH_CHECK(mergeCollections( m_inputLArEMECHits,         m_outputLArEMECHits         ));
+  ATH_CHECK(mergeCollections( m_inputLArFCALHits,         m_outputLArFCALHits         ));
+  ATH_CHECK(mergeCollections( m_inputLArHECHits,          m_outputLArHECHits          ));
 
-  mergeCollections( m_inputTileHits,            m_outputTileHits            );
-  mergeCollections( m_inputMBTSHits,            m_outputMBTSHits            );
+  ATH_CHECK(mergeCollections( m_inputTileHits,            m_outputTileHits            ));
+  ATH_CHECK(mergeCollections( m_inputMBTSHits,            m_outputMBTSHits            ));
 
-  mergeCollections( m_inputCSCHits,             m_outputCSCHits             );
-  mergeCollections( m_inputMDTHits,             m_outputMDTHits             );
-  mergeCollections( m_inputRPCHits,             m_outputRPCHits             );
-  mergeCollections( m_inputTGCHits,             m_outputTGCHits             );
+  ATH_CHECK(mergeCollections( m_inputCSCHits,             m_outputCSCHits             ));
+  ATH_CHECK(mergeCollections( m_inputMDTHits,             m_outputMDTHits             ));
+  ATH_CHECK(mergeCollections( m_inputRPCHits,             m_outputRPCHits             ));
+  ATH_CHECK(mergeCollections( m_inputTGCHits,             m_outputTGCHits             ));
 
   return StatusCode::SUCCESS;
 }
@@ -186,6 +186,6 @@ StatusCode ISF::CollectionMerger::finalize()
 StatusCode ISF::CollectionMerger::initializeVarHandleKey( SG::VarHandleKey& varHandleKey ) const {
   if ( varHandleKey.key().empty() )
     return StatusCode::SUCCESS;
-  
+
   return varHandleKey.initialize();
 }

Simulation/ISF/ISF_Core/ISF_Algorithms/src/CollectionMerger.h

@@ -81,8 +81,17 @@ namespace ISF {
 
     /** Merge all hits of inputReadHandleKeys's collections into outputWriteHandleKey */
     template <typename T>
-    void mergeCollections( const ReadHandleKeyVector_t<T>& inputReadHandleKeys,
-                           SG::WriteHandleKey<T>&          outputWriteHandleKey ) const;
+    StatusCode mergeCollections( const ReadHandleKeyVector_t<T>& inputReadHandleKeys,
+                                 SG::WriteHandleKey<T>& outputWriteHandleKey ) const;
+
+	/** Copy the given hit into the given output collection, container or DataHandle */
+	template <typename HitType_t, typename OutputType_t>
+	void insertCopy(const HitType_t& hit, OutputType_t& outputHandle) const;
+
+	/** Copy the given const pointer to a hit into the given output collection,
+	    container or DataHandle */
+	template <typename HitType_t, typename OutputType_t>
+	void insertCopy(HitType_t * const hit, OutputType_t& outputHandle) const;
 
     /** Input collection StoreGate keys */
     SGKeyVector_t                                       m_inputBCMHitsSGKeys;
@@ -145,7 +154,6 @@ namespace ISF {
     SG::WriteHandleKey<TGCSimHitCollection>             m_outputTGCHits;
   };
 
-}
 
 //
 // templated methods below
@@ -175,28 +183,48 @@ StatusCode ISF::CollectionMerger::setupReadHandleKeyVector( const SGKeyVector_t&
 
 /** Merge all hits of inputReadHandleKeys's collections into outputWriteHandleKey */
 template <typename T>
-void ISF::CollectionMerger::mergeCollections( const ReadHandleKeyVector_t<T>& inputReadHandleKeys,
-                                              SG::WriteHandleKey<T>&          outputWriteHandleKey ) const {
+inline StatusCode ISF::CollectionMerger::mergeCollections( const ReadHandleKeyVector_t<T>& inputReadHandleKeys,
+                                              SG::WriteHandleKey<T>& outputWriteHandleKey ) const {
   // skip if not input collection
   if ( inputReadHandleKeys.empty() ) {
-    return;
+    return StatusCode::SUCCESS;
   }
   // TODO: is there a way to conveniently get the total number of hits in all inputReadHandleKeys
   //       and reserve the corresponding size in the outputHandle
-  SG::WriteHandle<T>  outputHandle{outputWriteHandleKey};
-  if (outputHandle.record( CxxUtils::make_unique<T>() ).isFailure()) {
-    return;
-  }
+  SG::WriteHandle<T> outputHandle{outputWriteHandleKey};
+  ATH_CHECK( outputHandle.record(std::make_unique<T>()) );
 
   for ( const auto& collKey: inputReadHandleKeys ) {
-    SG::ReadHandle<T>  inputHandle{collKey};
+    SG::ReadHandle<T> inputHandle{collKey};
 
     for ( const auto& hit: *inputHandle ) {
-      // TODO: replace with ->Emplace(hit) once LArHitContainer supports this
-      outputHandle->push_back( hit );
+      this->insertCopy(hit, outputHandle);
     }
   }
+
+  return StatusCode::SUCCESS;
+}
+
+
+/** Copy the given hit into the given output collection, container or DataHandle */
+template <typename HitType_t, typename OutputType_t>
+inline void ISF::CollectionMerger::insertCopy(const HitType_t& hit,
+                                           OutputType_t& outputHandle) const {
+  static_assert(!std::is_pointer<HitType_t>::value,
+                "The hit provided to ISF::CollectionMerger::insertCopy(..) must not be a pointer!");
+  outputHandle->Emplace( hit );
 }
 
 
+/** Copy the given const pointer to a hit into the given output collection, container or DataHandle */
+template <typename HitType_t, typename OutputType_t>
+inline void ISF::CollectionMerger::insertCopy(HitType_t * const hit,
+                                           OutputType_t& outputHandle) const {
+  auto&& hitCopy = std::make_unique<HitType_t>(*hit);
+  outputHandle->push_back( hitCopy.release() );
+}
+
+
+}
+
 #endif //> !ISF_ALGS_COLLECTIONMERGER_H

Simulation/ISF/ISF_Core/ISF_Algorithms/test/CollectionMerger_test.cxx

@@ -31,14 +31,17 @@ namespace ISFTesting {
 // needed every time an AthAlgorithm, AthAlgTool or AthService is instantiated
 ISvcLocator* g_svcLoc = nullptr;
 
-// mock object for sensitive detector hits
-class TestHits {
+
+// fake sensitive detector hits
+class TestHit {
   public:
-    TestHits(int value):m_value(value) { };
+    TestHit(int value):m_value(value) { };
     int m_value;
 };
+
 // typedef for convenience
-typedef AtlasHitsVector<ISFTesting::TestHits> TestHitCollection_t;
+typedef AtlasHitsVector<ISFTesting::TestHit> TestHitCollection_t;
+typedef AthenaHitsVector<ISFTesting::TestHit> TestPointerHitCollection_t;
 
 
 // global test environment takes care of setting up Gaudi
@@ -196,7 +199,7 @@ TEST_F(CollectionMerger_test, mergeCollections) {
   ASSERT_TRUE( mergedCollectionHandle.isValid() );
   ASSERT_EQ( mergedCollectionHandle->size(), 3+2+3 );
 
-  auto& mergedCollectionVector = mergedCollectionHandle->getVector();
+  const auto& mergedCollectionVector = mergedCollectionHandle->getVector();
   ASSERT_EQ( mergedCollectionVector.at(0).m_value, 1  ); // inputCollectionA
   ASSERT_EQ( mergedCollectionVector.at(1).m_value, 20 ); // inputCollectionA
   ASSERT_EQ( mergedCollectionVector.at(2).m_value, 5  ); // inputCollectionA
@@ -250,7 +253,7 @@ TEST_F(CollectionMerger_test, integration_with_data) {
   ASSERT_TRUE( mergedCollectionHandle.isValid() );
   ASSERT_EQ( mergedCollectionHandle->size(), 3+2+3 );
 
-  auto& mergedCollectionVector = mergedCollectionHandle->getVector();
+  const auto& mergedCollectionVector = mergedCollectionHandle->getVector();
   ASSERT_EQ( mergedCollectionVector.at(0).trackNumber(), 1  ); // inputPixelCollectionIntegrationTestA
   ASSERT_EQ( mergedCollectionVector.at(1).trackNumber(), 20 ); // inputPixelCollectionIntegrationTestA
   ASSERT_EQ( mergedCollectionVector.at(2).trackNumber(), 5  ); // inputPixelCollectionIntegrationTestA
@@ -262,10 +265,141 @@ TEST_F(CollectionMerger_test, integration_with_data) {
 }
 
 
+TEST_F(CollectionMerger_test, one_empty_one_filled_input_collection___expect_filled_output_collection) {
+  std::string inputPropertyValue = "['inputPixelCollectionIntegrationTestFilled',"
+                                    "'inputPixelCollectionIntegrationTestEmpty']";
+  std::string outputPropertyValue = "'outputPixelCollectionTestFilled'";
+  ASSERT_TRUE( m_alg->setProperty( "InputPixelHits",  inputPropertyValue).isSuccess()  );
+  ASSERT_TRUE( m_alg->setProperty( "OutputPixelHits", outputPropertyValue).isSuccess() );
+
+  // create dummy input collections containing dummy data
+  HepGeom::Point3D<double> pos(0.,0.,0.);
+  SG::WriteHandle<SiHitCollection> inputTestDataHandleFilled{"inputPixelCollectionIntegrationTestFilled"};
+  ASSERT_TRUE(inputTestDataHandleFilled.record(CxxUtils::make_unique<SiHitCollection>()).isSuccess());
+  inputTestDataHandleFilled->Emplace( pos, pos, 1., 1.,  1, 0 );
+  inputTestDataHandleFilled->Emplace( pos, pos, 1., 1., 20, 0 );
+  inputTestDataHandleFilled->Emplace( pos, pos, 1., 1.,  5, 0 );
+
+  SG::WriteHandle<SiHitCollection> inputTestDataHandleEmpty{"inputPixelCollectionIntegrationTestEmpty"};
+  ASSERT_TRUE(inputTestDataHandleEmpty.record(CxxUtils::make_unique<SiHitCollection>()).isSuccess());
+
+  ASSERT_TRUE( m_alg->initialize().isSuccess() );
+  ASSERT_TRUE( m_alg->execute().isSuccess() );
+
+  // test "outputPixelCollectionIntegrationTest" contents
+  SG::ReadHandleKey<SiHitCollection>  mergedCollectionKey{"outputPixelCollectionTestFilled"};
+  ASSERT_TRUE( mergedCollectionKey.initialize().isSuccess() );
+  SG::ReadHandle<SiHitCollection> mergedCollectionHandle{mergedCollectionKey};
+
+  ASSERT_TRUE( mergedCollectionHandle.isValid() );
+  ASSERT_EQ( 3, mergedCollectionHandle->size() );
+
+  const auto& mergedCollectionVector = mergedCollectionHandle->getVector();
+  ASSERT_EQ( mergedCollectionVector.at(0).trackNumber(), 1  ); // inputPixelCollectionIntegrationTestA
+  ASSERT_EQ( mergedCollectionVector.at(1).trackNumber(), 20 ); // inputPixelCollectionIntegrationTestA
+  ASSERT_EQ( mergedCollectionVector.at(2).trackNumber(), 5  ); // inputPixelCollectionIntegrationTestA
+}
+
+
+TEST_F(CollectionMerger_test, preexisting_output_collection___expect_execute_isFailure) {
+  std::string inputPropertyValue = "['inputPixelCollectionTestX']";
+  std::string outputPropertyValue = "'outputPixelCollectionTestPreexisting'";
+  ASSERT_TRUE( m_alg->setProperty( "InputPixelHits",  inputPropertyValue).isSuccess()  );
+  ASSERT_TRUE( m_alg->setProperty( "OutputPixelHits", outputPropertyValue).isSuccess() );
+
+  // create dummy input collections containing dummy data
+  SG::WriteHandle<SiHitCollection> inputTestDataHandleA{"inputPixelCollectionTestX"};
+  ASSERT_TRUE( inputTestDataHandleA.record(CxxUtils::make_unique<SiHitCollection>()).isSuccess() );
+
+  // create pre-existing output collection
+  SG::WriteHandle<SiHitCollection> outputDataHandle{"outputPixelCollectionTestPreexisting"};
+  ASSERT_TRUE( outputDataHandle.record(CxxUtils::make_unique<SiHitCollection>()).isSuccess() );
+
+  ASSERT_TRUE( m_alg->initialize().isSuccess() );
+  ASSERT_TRUE( m_alg->execute().isFailure() );
+}
+
+
+TEST_F(CollectionMerger_test, nonexisting_input_collection___expect_SG_exception) {
+  std::string inputPropertyValue = "['inputPixelCollectionDoesntExist']";
+  std::string outputPropertyValue = "'outputPixelCollectionTest123'";
+  ASSERT_TRUE( m_alg->setProperty( "InputPixelHits",  inputPropertyValue).isSuccess()  );
+  ASSERT_TRUE( m_alg->setProperty( "OutputPixelHits", outputPropertyValue).isSuccess() );
+
+  ASSERT_TRUE( m_alg->initialize().isSuccess() );
+  ASSERT_THROW( m_alg->execute(), SG::ExcNullReadHandle );
+}
+
+
+TEST_F(CollectionMerger_test, mergeCollections_with_pointer_types___expect_merged_pointers_not_equal_to_input) {
+  std::vector<SG::ReadHandleKey<TestPointerHitCollection_t>> inputKeys{};
+  SG::WriteHandleKey<TestPointerHitCollection_t> outputKey{"outputPointerCollectionMergeTest"};
+  ASSERT_TRUE( outputKey.initialize().isSuccess() );
+
+  // create dummy input collections containing dummy data
+  auto inputTestDataA = CxxUtils::make_unique<TestPointerHitCollection_t>();
+  SG::WriteHandle<TestPointerHitCollection_t> inputTestDataHandleA{"inputPointerCollectionA"};
+  inputTestDataHandleA.record( std::move(inputTestDataA) );
+  auto* inputHitA1 = new ISFTesting::TestHit(1);
+  inputTestDataHandleA->push_back(inputHitA1);
+  auto* inputHitA2 = new ISFTesting::TestHit(20);
+  inputTestDataHandleA->push_back(inputHitA2);
+  auto* inputHitA3 = new ISFTesting::TestHit(5);
+  inputTestDataHandleA->push_back(inputHitA3);
+
+  auto inputTestDataB = CxxUtils::make_unique<TestPointerHitCollection_t>();
+  SG::WriteHandle<TestPointerHitCollection_t> inputTestDataHandleB{"inputPointerCollectionB"};
+  inputTestDataHandleB.record( std::move(inputTestDataB) );
+  auto* inputHitB1 = new ISFTesting::TestHit(50);
+  inputTestDataHandleB->push_back(inputHitB1);
+  auto* inputHitB2 = new ISFTesting::TestHit(1);
+  inputTestDataHandleB->push_back(inputHitB2);
+
+  // add inputCollections with test data to inputKeys for later merging
+  // ordering A, C, B is on purpose to test for unintended alphabetic ordering
+  inputKeys.emplace_back( "inputPointerCollectionA" );
+  inputKeys.emplace_back( "inputPointerCollectionB" );
+  ASSERT_TRUE( inputKeys.at(0).initialize().isSuccess() );
+  ASSERT_TRUE( inputKeys.at(1).initialize().isSuccess() );
+
+  // merge pointer collections
+  mergeCollections(inputKeys, outputKey);
+
+  // test "outputCollectionMergeTest" contents
+  SG::ReadHandleKey<TestPointerHitCollection_t> mergedCollectionKey{"outputPointerCollectionMergeTest"};
+  ASSERT_TRUE( mergedCollectionKey.initialize().isSuccess() );
+  SG::ReadHandle<TestPointerHitCollection_t> mergedCollectionHandle{mergedCollectionKey};
+
+  ASSERT_TRUE( mergedCollectionHandle.isValid() );
+  ASSERT_EQ( 3+2, mergedCollectionHandle->size() );
+
+  const auto* outputHit1 = (*mergedCollectionHandle)[0];  // inputPointerCollectionA
+  const auto* outputHit2 = (*mergedCollectionHandle)[1];  // inputPointerCollectionA
+  const auto* outputHit3 = (*mergedCollectionHandle)[2];  // inputPointerCollectionA
+  const auto* outputHit4 = (*mergedCollectionHandle)[3];  // inputPointerCollectionB
+  const auto* outputHit5 = (*mergedCollectionHandle)[4];  // inputPointerCollectionB
+
+  // check merged hit pointers are different from input (i.e. expect copies, not identities of input in output)
+  ASSERT_NE( inputHitA1, outputHit1 );  // inputPointerCollectionA
+  ASSERT_NE( inputHitA2, outputHit2 );  // inputPointerCollectionA
+  ASSERT_NE( inputHitA3, outputHit3 );  // inputPointerCollectionA
+  ASSERT_NE( inputHitB1, outputHit4 );  // inputPointerCollectionB
+  ASSERT_NE( inputHitB2, outputHit5 );  // inputPointerCollectionB
+
+  // check values of hits are copied
+  ASSERT_EQ( outputHit1->m_value, 1  );  // inputPointerCollectionA
+  ASSERT_EQ( outputHit2->m_value, 20 );  // inputPointerCollectionA
+  ASSERT_EQ( outputHit3->m_value, 5  );  // inputPointerCollectionA
+  ASSERT_EQ( outputHit4->m_value, 50 );  // inputPointerCollectionB
+  ASSERT_EQ( outputHit5->m_value, 1  );  // inputPointerCollectionB
+}
+
+
 } // <-- namespace ISFTesting
 
 
 CLASS_DEF (ISFTesting::TestHitCollection_t, 1234567890, 1)
+CLASS_DEF (ISFTesting::TestPointerHitCollection_t, 1234567891, 1)
 
 
 int main(int argc, char *argv[])