[WiP] Make sure to always only have valid collection names

[tmadlener]

BEGINRELEASENOTES

• Make sure to properly clean up collection names when they are extracted from the Gaudi TES

ENDRELEASENOTES

Fixes #235

• Tests
• Needs [WIP] Add a service to allow retrieving collections to which an object belongs k4FWCore#312
• Needs Make sure to not double convert link collections #237
• Includes Move duplicated code into helper function #239
• Includes Refactor retrieval of EDM4hep event Frame into method #238
• Includes Add missing newlines for debug output #243

@Zehvogel can you give this a go to see if it doesn't break other things?

[Zehvogel]

Lol, now it gets further than before, but still fails in the same line, this time hitting this exception:
https://github.com/iLCSoft/LCIO/blob/96e6a070780b94237ac7728c96b05fd664edd128/src/cpp/src/UTIL/PIDHandler.cc#L102-L103
I added a debug message to check and it still chokes on the BuildUpVertices collection, now correctly spelt...
So I cannot say yet if this PR will break anything :D

[tmadlener]

I have switched the name lookup away from requiring the collection ID, so I think we should no longer mix up collections during the conversion. I didn't fully test this yet, but it should in principle fix the issue.

@Zehvogel can you run some quick checks with this? Or alternatively let me know which branch of CLDConfig to use to test myself.

[Zehvogel]

@tmadlener you can check it with the MLJetTagger test in key4hep/CLDConfig#78

I can probably take a look this afternoon, but maybe you are faster ;)

[Zehvogel]

Now I am suspicious about:

[ctest] ToolSvc.EventNu...  DEBUG Event: 0 Run: 0
[ctest] EventNumber         FATAL Standard std::exception is caught in sysExecute
[ctest] EventNumber         ERROR lcio::DataNotAvailableException: LCEventImpl::getCollection: collection not in event:VertexJets_PID_yth

In the LCIO world, VertexJets_PID_yth should not be a collection, so this gets wrongly picked up somewhere...

[tmadlener]

Hmm... Looks like some of the ParticleID collections now escape their enclosure somehow. Let me investigate why.

[tmadlener]

OK. I found the issue with the current version. Before my "fix" of not using the collection IDs, we were actually getting the name of the reconstructed particle collection from the first element of the particle id collection. With the changes in place we skip that lookup and rather use the collection name of the particle id, which is obviously wrong. This also means that the work around for not having to use the object id doesn't work as easily as I had thought and we probably need to address key4hep/k4FWCore#311 first.

[tmadlener]

I have switched back to using the collection ID to identify collections to which ParticleID information should be attached. This means that we need the changes in key4hep/k4FWCore#312, especially since we make direct use of one of the newly introduced methods there.

With the latest changes this also switches the storage of converted EDM4hep collections (from EDM4hep) to the TES over from the (to be deprecated) DataHandle to use AnyDataWrapper, which immediately makes it compatible with Functional algorithms and the IOSvc. Depending on where this PR ends up, we might want to split that off into a separate PR.

This does still not fix all the issues with CLD reconstruction. There is a mixup / duplication of PID algorithms now:

ToolSvc.EventNu...  DEBUG Using ca369aad as collection id to lookup LCIO collection for attaching ParticleID metadata
ToolSvc.EventNu...  DEBUG Corresponding name in EDM4hep is: VertexJets
ToolSvc.EventNu...  DEBUG LCIO collection has type: ReconstructedParticle
EventNumber         FATAL Standard std::exception is caught in sysExecute
EventNumber         ERROR lcio::Exception:  PIDHandler::addAlgorithm() - duplicate algorithm name: yth
k4FWCore__Algs      FATAL Standard std::exception is caught in sysExecute
k4FWCore__Algs      ERROR lcio::Exception:  PIDHandler::addAlgorithm() - duplicate algorithm name: yth
k4FWCore__Seque...  FATAL Standard std::exception is caught in sysExecute
k4FWCore__Seque...  ERROR lcio::Exception:  PIDHandler::addAlgorithm() - duplicate algorithm name: yth
EventLoopMgr        FATAL .executeEvent(): Standard std::exception thrown by k4FWCore__Sequencer
EventLoopMgr        ERROR lcio::Exception:  PIDHandler::addAlgorithm() - duplicate algorithm name: yth

[Zehvogel]

This does still not fix all the issues with CLD reconstruction. There is a mixup / duplication of PID algorithms now:

Oh, is there just no check if a PID already exists before converting it to LCIO?

Indeed, I think here we need to make another distinction, as the PID is in collections in edm4hep that never will be in lcio because there PID info is not stored as a collection:
https://github.com/tmadlener/k4MarlinWrapper/blob/20386aaa9b64b4cc8ad7e55e0b40f692ca81f57f/k4MarlinWrapper/src/components/EDM4hep2Lcio.cpp#L436-L439

    debug() << "Converting collection " << edm4hepName << " (storing it as " << lcioName << ")" << endmsg;
    if (!EDM4hep2LCIOConv::collectionExist(lcioName, lcio_event)) {
      convertAdd(edm4hepName, lcioName, lcio_event, collection_pairs, pidCollections, dQdxCollections);
    } else {

[Zehvogel]

Probably, we want to make that distinction already earlier, were we just add all the collections to be converted here
https://github.com/tmadlener/k4MarlinWrapper/blob/20386aaa9b64b4cc8ad7e55e0b40f692ca81f57f/k4MarlinWrapper/src/components/EDM4hep2Lcio.cpp#L397-L420

[tmadlener]

Oh, is there just no check if a PID already exists before converting it to LCIO?

The problem here is different, I think. We split PIDs into different collections on a "PID algorithm" base. Hence, there will be several PIDs with the same algorithm but different reco collections. I would guess that we mix up things here somewhere and then try to assign to the wrong reco collection.

[tmadlener]

I seem to have solved all issues locally to make key4hep/CLDConfig#78 work (with some minor adaptions to that). I will have to check what I broke in the process and looking at the convertCollections function that came out of this, I also think some refactoring of that is still in order.