Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jun 11, 2024

Conversation

tmadlener
Copy link
Contributor

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 Stable / consistent algorithm types for PIDHandler #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.

@hegner hegner self-requested a review May 17, 2024 12:44
@jmcarcell
Copy link
Member

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

@tmadlener
Copy link
Contributor Author

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
Copy link
Member

jmcarcell commented May 22, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Unless there are any more comments, I would merge this sometime tomorrow (synchronously with the necessary API changes in key4hep/k4EDM4hep2LcioConv#68)

@tmadlener tmadlener merged commit 2ee9646 into key4hep:main Jun 11, 2024
8 of 11 checks passed
@tmadlener tmadlener deleted the hash-algo-id branch June 11, 2024 08:02
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.

Stable / consistent algorithm types for PIDHandler
3 participants