search

key4hep / k4FWCore

Core Components for the Gaudi-based Key4hep Framework
Apache License 2.0
10 stars 26 forks source link

Use `std::vector` instead of `std::map` for reading or writing an arbitrary number of collections. #201

Closed jmcarcell closed 2 months ago

jmcarcell commented 2 months ago

The current implementation with std::map is not easy to use. The collections get ordered alphabetically in std::map so the order set in the steering file is lost, which means if there is some kind of mapping (like one output collection for each input collection) then one has to reproduce this map in the algorithm and the way I found to do it is to have another property with the input and output names again. For output one creates the collections and then puts them in a vector, I think it can't get simpler than that. For input, since there can't be vectors to references one gets a pointer to the collection, which is not ideal but not so bad. With a custom type it would be possible to have a vector that when using [] can get a reference but then that introduces a custom type which I don't like.

BEGINRELEASENOTES

ENDRELEASENOTES

jmcarcell commented 2 months ago

I had a quick look. There are still a few occurrences of 'map' where 'vector' should be used

Functions detail::readMapInputs and detail::putMapOutputs could also be renamed since they operate on vectors

I think const * are acceptable

In case the information about collection keys will be needed: In Gaudi there isinputLocation() and outputLocation() (defined in DataHandleMixing) that can be used to get keys of inputs and outputs (like tuple either by index or type if unique). From what I seen they are not accessible from k4FWCore functional algorithms. It might be good idea to add something similar here. Alternatively something like std::get<n>(m_outputs).at(m).fullKey().key() could be used to get key of n-th output and m-th collection (given n-th output is a vector of collections)

There is also m_inputLocations and m_outputLocations that can be used like here: https://github.com/key4hep/k4Reco/pull/2/files#diff-fd32a841abe95600867a31e1e8a8f06232c5fe7e654319fdfad0d14b55760bc0R256. I don't have a strong opinion on implementing inputLocation() and outputLocation() or not.

jmcarcell commented 2 months ago

I added a inputLocations and outputLocations to retrieve those and they are used in a couple of tests. Now I notice the Gaudi version is in singular but here it can be more than one. I think them being views is fine, but they can easily be changed to a vector.

jmcarcell commented 2 months ago

Any more comments or comments about the functions to get the locations? @m-fila

m-fila commented 2 months ago

Any more comments or comments about the functions to get the locations? @m-fila

Sorry I was busy with other things, I'll take another look

jmcarcell commented 2 months ago

I'll merge this later today, #209 has to be rebased on top of this. Note to myself: fix formatting, clang-format with Clang 18 gives different results from what you get from the nightlies...