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.

Sign up for GitHub

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

Jump to bottom

Add the possibility to set edm4hep::utils::ParticleIDMeta via the IMetadataSvc #273

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Add the possibility to set edm4hep::utils::ParticleIDMeta via the IMetadataSvc #273

wants to merge 11 commits into from

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Dec 20, 2024 •
edited
Loading

BEGINRELEASENOTES

  • Make it possible to use edm4hep::utils::ParticleIDMeta with the MetadataSvc
    • Add template specializations for get and put that defer to the corresponding utility calls in EDM4hep
    • Add tests to ensure that metadata is indeed usable this way with the utilities in EDM4hep

ENDRELEASENOTES

This would be option 1 sketched out in #272

@tmadlener tmadlener mentioned this pull request Jan 16, 2025
Open
2 tasks
@tmadlener
Copy link
Contributor Author

tmadlener commented Jan 16, 2025 •
edited
Loading

I have added a "small" test case that runs similar checks as in the functional algorithm as a standalone application (effectively hard-coding the expected parameter and algorithm names). I have done it in c++ because the PIDHandler is not yet really usable in python (see key4hep/EDM4hep#396).

Once the nightly builds have picked up key4hep/EDM4hep#395, CI should also start to pass again (tomorrow).

@tmadlener tmadlener changed the title [WIP] Add the possibility to set edm4hep::utils::ParticleIDMeta via the IMetadataSvc Add the possibility to set edm4hep::utils::ParticleIDMeta via the IMetadataSvc Jan 16, 2025
jmcarcell
jmcarcell reviewed Jan 17, 2025
View reviewed changes
test/k4FWCoreTest/src/components/ExampleParticleIDConsumer.cpp Outdated Show resolved Hide resolved
test/k4FWCoreTest/CMakeLists.txt


add_executable(check_ParticleIDOutputs src/check_ParticleIDOutputs.cpp)
target_link_libraries(check_ParticleIDOutputs PRIVATE podio::podioIO EDM4HEP::edm4hep EDM4HEP::utils fmt::fmt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be good to have a find_package(fmt)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good idea. I was relying on find_package(Gaudi) bringing it in. Should I put it into the top level CMakeLists.txt or in the one in tests? Currently only the tests seem to be using fmtlib, but that might change(?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well with #275 it will change, so it may be at the top level anyway.

test/k4FWCoreTest/src/check_ParticleIDOutputs.cpp Outdated Show resolved Hide resolved
test/k4FWCoreTest/src/check_ParticleIDOutputs.cpp Show resolved Hide resolved
@tmadlener
Make test functions consistent and check specific values
3803077
@tmadlener
Add license header
e4a1ff4
@tmadlener
Fix unrelated test failures
cc8a2af 
Necessary because there is a new output collection in the functional
producer that is used in several places
@tmadlener
Copy link
Contributor Author

Since I touched the ExampleFunctionalProducerMultiple to also create ReconstructedParticles, I had to fix a few other tests to also make them pass. The alternative would be to introduce a dedicated producer for the ParticleID metadata tests. I am not sure if it's worth it, but it can obviously be done.

@tmadlener
Find fmtlib in cmake if we build the tests
50d288a 
Technically already covered via Gaudi, but this makes it explicit
@tmadlener
Find fmtlib unconditionally
842a5d5
tmadlener
tmadlener commented Jan 20, 2025
View reviewed changes
test/k4FWCoreTest/src/components/ExampleParticleIDConsumer.cpp
void operator()(const edm4hep::ParticleIDCollection& pidColl1, const edm4hep::ParticleIDCollection& pidColl2,
const edm4hep::ReconstructedParticleCollection& recos) const {
auto pidHandler = edm4hep::utils::PIDHandler::from(pidColl1, pidColl2);
pidHandler.addMetaInfos(m_pidMeta1, m_pidMeta2);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires a new version of EDM4hep. Should I go for a version that works with 0.99? Otherwise we need to bump the EDM4hep version, I think. Even if this is only a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmcarcell jmcarcell jmcarcell left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tmadlener @jmcarcell