Switch LCIO to EDM4hep conversion to use k4EDM4hep2LcioConv functionality

[tmadlener]

BEGINRELEASENOTES

• Replace the k4LCIOReader based conversion from LCIO to EDM4hep with new functionality that has been added to k4EDM4hep2LCIOConv in Add LCIO to EDM4hep conversion functionality k4EDM4hep2LcioConv#11

ENDRELEASENOTES

• Needs Add LCIO to EDM4hep conversion functionality k4EDM4hep2LcioConv#11
• ParticleIDs associated to Clusters are not yet handled in converter
• Needs Fix DataHandle to work as expected when used with LegacyDataSvc k4FWCore#117

@Zehvogel you want to give this a go and test it?

[Zehvogel]

Thanks a lot Thomas, I will try it out and report back :)

[tmadlener]

I think I was able to significantly simplify this by exploiting more functionality already present in the conversion library. You will need to get the latest versions of both PRs for testing.

[Zehvogel]

At the moment I don't get very far before it segfaults

#4  0x00007f2fefade1a5 in TUnixSystem::DispatchSignals (this=0x16e3260, sig=kSigSegmentationViolation) at /tmp/root/spack-stage/spack-stage-root-6.26.10-yoe5qnbrwojxykb47wc4a2uco3vmn3mn/spack-src/core/unix/src/TUnixSystem.cxx:3615
#5  <signal handler called>
#6  0x00007f2fc3a839e2 in DataHandle<podio::CollectionBase>::put(podio::CollectionBase*) () from /home/lreichen/work/test8/spack/opt/spack/x86_64-almalinux9-gcc11.3.1-opt/k4marlinwrapper/master-2ill5i/lib64/libLcio2EDM4hep.so
#7  0x00007f2fc3a7cd47 in Lcio2EDM4hepTool::registerCollection(std::tuple<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::unique_ptr<podio::CollectionBase, std::default_delete<podio::CollectionBase> > >, EVENT::LCCollection*) () from /home/lreichen/work/test8/spack/opt/spack/x86_64-almalinux9-gcc11.3.1-opt/k4marlinwrapper/master-2ill5i/lib64/libLcio2EDM4hep.so
#8  0x00007f2fc3a7d8e2 in Lcio2EDM4hepTool::convertCollections(IMPL::LCEventImpl*) () from /home/lreichen/work/test8/spack/opt/spack/x86_64-almalinux9-gcc11.3.1-opt/k4marlinwrapper/master-2ill5i/lib64/libLcio2EDM4hep.so
#9  0x00007f2fd8f538c7 in MarlinProcessorWrapper:

should be reproducible by running

./01-00-sim_single_electrons.sh
./02-00-do_reco.sh

from https://github.com/Zehvogel/resolutions (might want to reduce the number of events)

[tmadlener]

Thanks for the scripts to easily reproduce this. It took me a bit but I tracked down the seg fault to the DataHandle (resp. the DataObjectHandle[Base]) that seems to not be properly initialized when used the way I wanted it to. Registering collections to the PodioDataSvc directly seems to do the trick though. I was able to run the two scripts without crashes now. I will leave the proper checking of the results to you ;)

[Zehvogel]

Thanks for the fast fix Thomas!
Unfortunately I think I still found multiple problems.

Simple things first:

Relation from: CalorimeterHit to: MCParticle (CalohitMCTruthLink) is not beeing handled during creation of associations

I guess that just needs one more if/else case.

During the reconstruction I also get what looks like one of these per particle in the event

Cannot find corresponding EDM4hep object for an LCIO object in a subset collection

additionally .index and .collectionID are always -2 for all entries in RecoMCTruthLink#{0,1}.

I think the problem is that typeMapping.mcParticles is empty as the MCParticles collection is (rightfully) not converted from LCIO to EDM4hep as it already exists there when using EDM4hep sim files.

[tmadlener]

Thanks for testing so quickly.

The CaloHit -> MCParticle association should indeed just be one more case to handle for the association creation.

Having a mapping of EDM4hep <-> LCIO MCParticles is something that is not entirely as easy to handle I think, since this is something that needs to be shared between converters in both directions, and it needs to be continuously updated for each conversion that happens. I have to check if that is even possible with GaudiTools. If that is possible that should fix the case we have here, since there is one complete conversion of the sim input to LCIO, right?

[Zehvogel]

Yes, there is one complete sim input conversion to LCIO in the beginning.

For the simple case now with one LCIO to EDM4hep conversion in the beginning and one EDM4hep to LCIO conversion in the end, I do not get the part about any need of updating. The main MCParticle collection (MCParticles) should not change and all other MCParticle collections should be subset collections. Is it not enough to read in the EDM4hep MCParticles collection to populate the mapping at the time of the LCIO to EDM4hep conversion?

However there might(will?) be a need for synchronisation if I want to call a regular EDM4hep Gaudi algorithm between a bunch of MarlinWrapper ones?

[tmadlener]

Is it not enough to read in the EDM4hep MCParticles collection to populate the mapping at the time of the LCIO to EDM4hep conversion?

It would be, but that is not what is currently happening. At the moment each of the individual converters has its own mapping that is not shared with any of the others. Additionally, the mapping structures are slightly different for the two directions. So what we need is

• One mapping that works in both directions (for which we probably have to drop some of the constness that we can currently embed in there)
• Share that mapping between all converters (for which I am not entirely sure whether we really want this, or whether we have to foresee a mode where we can "isolate" a converter from the rest)

I hope there is a possibility to transport the information via the TES, otherwise we will have to fall back to some static content here. In any case neither of the approaches will allow for running this on multiple threads without quite a bit of synchronization work.

[Zehvogel]

@tmadlener So far I noticed no other obvious bugs. Can we get this merged and do the synchronisation later?

[tmadlener]

From what I gathered today CI is failing because:

• Problems introduced in various "quick fix" PRs, that should be fixed by Fix DataHandle to work as expected when used with LegacyDataSvc k4FWCore#117 and Fix formatting and usage of Legacy components in tests #118
• There seems to be a problem in one of the relations in the PandoraPFOs collection, where one of the tracks seems to point to a track outside a collection. Given that tracks are created in LCIO, I think relations should be OK. I am not entirely sure what is going on there.

[tmadlener]

@Zehvogel @andresailer @jmcarcell how do we go forward with this? CI is horribly broken and fixing it might require some work (see #119 ). On the other hand fixing all of that would probably be easier on top of these changes. Do we merge this now (and potentially #118 to at least have some tests back alive)? Or do we want one PR that fixes everything? (Not sure yet how big that would become, or how easy it actually is to get there)

[Zehvogel]

I would be in favour of merging this (and #118) quickly and continue fixing from there. Merging this will not make anything more broken, right?

[tmadlener]

Merging this will not make anything more broken, right?

It shouldn't but at this point is not really easy to tell. This seemed to at least have worked for you and for me locally, so I suppose it should be OK. #118 is only fixes, although it doesn't fix everything yet.

[tmadlener]

Merging this after at least the format check has passed as it is working in manual tests and we need it for #120

[tmadlener]

Something is still off with the edm_converters test that should be fixed here, rather than in a follow up PR, I think. working on it

[Zehvogel]

Hm yes it would be good if that one works. Even better if that test had any meaningful output at all on what goes wrong... :D

[tmadlener]

I am not entirely sure why the anajob outputs changed on a seemingly "structural" level, where even things that do not have anything to do with the event content are different now. There are also some differences in the first event in any case, so updating the .expected files was necessary in any case. Technically this should make things run again in CI, although the converter is complaining quite a bit now, due to the underlying issue in #113