Use MurmurHash3 to hash the algorithm name for the algorithm type in ParticleIDMeta

key4hep / EDM4hep

Generic event data model for HEP collider experiments

https://cern.ch/edm4hep

Apache License 2.0

25 stars 35 forks source link

Use MurmurHash3 to hash the algorithm name for the algorithm type in ParticleIDMeta #307

Closed tmadlener closed 3 months ago

tmadlener commented 4 months ago

BEGINRELEASENOTES

To have a more consistent way of filling the algoType in the edm4hep::utils::ParticleIDMeta use the 32 bit version of MurmurHash3 to hash the algoName to get to algoType. Fixes #298
- Make algoType a private member but allow read access to it via the algoType member function. algoType has to be filled on construction. It is still possible to set it manually, the hashing will only kick in for the constructor taking only a name.

ENDRELEASENOTES

We could consider making ParticleID::algoType a uint32_t field for this, as that would make it a bit more easily visible that this is potentially a hash.

jmcarcell commented 4 months ago

Copying the murmurhash files every time we want to hash something isn't great...

tmadlener commented 4 months ago

I agree, but I am not sure where to put them for EDM4hep, and I don't really want to expose them from podio.

jmcarcell commented 4 months ago

A possibility could be to make a new Murmurhash package and build that library. Can any other hashes be considered, like boost::hash or is that a no no? The hashes here are saved in files as metadata right?

There is already a spack package for Murmurhash, but the main purpose seems to be its python bindings, we would probably need some CMake changes: https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/py-murmurhash/package.py to make it findable

tmadlener commented 4 months ago

I don't have a strict preference for any hashing algorithm. In the end the main requirement is that it has to fit into 32 bits. Whether we want to pull in boost just for that is another question though.

tmadlener commented 3 months ago

Unless there are any more comments, I would merge this sometime tomorrow (synchronously with the necessary API changes in https://github.com/key4hep/k4EDM4hep2LcioConv/pull/68)