-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add support for running with IOSvc #216
Conversation
cb3c7a3
to
5967d0b
Compare
for (const auto& pidCollMeta : pidCollections) { | ||
const auto algoId = attachParticleIDMetaData(lcio_event, edmEvent, pidCollMeta); | ||
auto algoId = attachParticleIDMetaData(lcio_event, edmEvent.value(), pidCollMeta); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this also needs a similar treatment as convertObjectParameters
in k4EDM4hep2LcioConv? That would make it possible to simplify the code below a bit, but I am not sure if it's worth the effort.
@@ -64,6 +67,9 @@ class EDM4hep2LcioTool : public AlgTool, virtual public IEDMConverter { | |||
|
|||
PodioDataSvc* m_podioDataSvc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have a one line description for each service under what circumstances which service is used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the ones I added?
I just realised I made a mistake and key4hep/k4EDM4hep2LcioConv#102 is not needed, so it can be reverted. key4hep/k4EDM4hep2LcioConv#102 was needed before because I was incorrectly writing the event parameters to the metadata frame. I discovered a limitation (added in a warning) of the current implementation: when running Marlin processors with |
This is mainly (only?) relevant for the LCIO to EDM4hep direction, right? As far as I can tell, the tests are unchanged apart from the additional infrastructure to switch between IOSvc and PodioDataSvc. In that case I would say this is OK for now and we check again in the future if someone runs into problems with this. |
I have edited the first post with information about all the tests. I think after key4hep/k4FWCore#280 functional algorithms and IOSvc should play well with the Marlin wrapper unless I'm missing any situation that is not in the tests... |
This is finally passing the CI (nightiles) so it's ready from my side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just one minor question, but since CI is passing, I think it's just me missing something.
We should also bump (or add in the first place) the minimum required version for k4FWCore in the top level CMakeLists.txt. But we need a new tag for that first for k4FWCore. |
CI passed in the previous run. The nightlies don't have all the tags to make it pass until tomorrow. |
BEGINRELEASENOTES
IOSvc
with a few tests:IOSvc
(proves processors run under IOSvc, producing the same output).global_converter_maps
withIOSvc
, where a functional algorithm produces output that is used by a Marlin processor and the output of the two is used either by aGaudi::Algorithm
or a functional algorithm (proves processors can take input from functional algorithms and functional algorithms can take input from processors).test_link_conversion_edm4hep
withIOSvc
, where links produced by functional algorithms are checked by Marlin processors.ENDRELEASENOTES
edm4hep::utils::ParticleIDMeta
via theIMetadataSvc
k4FWCore#273Fix #202