Skip to content

Set the CellID encoding parameters at finalize in Lcio2EDM4hep.cpp - #478

Merged
tmadlener merged 4 commits into
mainfrom
fill-map-encodings
Apr 29, 2026
Merged

Set the CellID encoding parameters at finalize in Lcio2EDM4hep.cpp#478
tmadlener merged 4 commits into
mainfrom
fill-map-encodings

Conversation

@jmcarcell

@jmcarcell jmcarcell commented Apr 22, 2026

Copy link
Copy Markdown
Member

BEGINRELEASENOTES

  • Set the CellID encoding parameters at finalize in Lcio2EDM4hep.cpp

ENDRELEASENOTES

Since due to 4c89e08c2a29b4c4b3c82bac06a59797faf2dd82 in k4FWCore, it is not possible to set parameters in the event loop anymore. I left the PodioDataSvc path as it is, since it will be removed soon.

@tmadlener

Copy link
Copy Markdown
Member

The problem is that we truly need these during the event loop. So we would probably need to front-load these. However, that would mean that we would need to discover all of them in initialize (e.g. by opening the LCIO file and reading the first few events to check).

The other approach is to follow what is currently done in #477 (but more general, along the discussion of key4hep/k4FWCore#301 (comment)

This whole thing with the small conceptual differences start to look to me like something that we might be more easily able to solve outside the framework (like we do for getting the patch collections) and simply pass in the necessary information from the outside such that we can inject it where necessary up-front, rather than figuring it out along the way (and hacking core principles of the framework while doing so).

@jmcarcell

Copy link
Copy Markdown
Member Author

Yes, I didn't see it because the metadata service is being used directly in a few places instead of going through k4FWCore::put or get. In CI it didn't get there because apparently the state is running when finalize() is called, and I think that is due to how the Marlin wrapper runs because I can not reproduce that. Maybe finding it in initialize is simpler? Only the first event would need to be checked I think. Otherwise they would have to be passed through python

@jmcarcell jmcarcell closed this Apr 22, 2026
@tmadlener

Copy link
Copy Markdown
Member

Only the first event would need to be checked I think. Otherwise they would have to be passed through python

In principle yes, but then again, depending on where you start from it's possible that the first event doesn't have all the collections (because that is not enforced in LCIO), so if you want to be sure you need to check a few at least (or potentially all). So having to manually set these from python seems like an acceptable trade-off (making that population automatic can then be done in python for example, or simply reading in a file like we do for patching collections).

Both CLD and ILD do that already in their reconstructions.

jmcarcell and others added 4 commits April 29, 2026 10:04
Since due to 4c89e08c2a29b4c4b3c82bac06a59797faf2dd82 in k4FWCore, it
is not possible to set parameters in the event loop anymore.
This reverts commit cdc1109.
@tmadlener
tmadlener force-pushed the fill-map-encodings branch from 1c48113 to 082db26 Compare April 29, 2026 08:04
@tmadlener

Copy link
Copy Markdown
Member

I have tested the ILD reconstruction and the CLD reconstruction with the canonical 3 ttbar events, both with LCIO and EDM4hep inputs (and also EDM4hep & LCIO outputs for ILD). All of these options run without failure now at least. I did not check the contents in much detail, but I would assume they should be similar (if not the same) as before as there are no apparent failures.

@tmadlener
tmadlener merged commit cb9b31c into main Apr 29, 2026
4 of 7 checks passed
@tmadlener
tmadlener deleted the fill-map-encodings branch April 29, 2026 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants