-
Notifications
You must be signed in to change notification settings - Fork 36
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
edm4hep2json: All EDM4hep collections + ROOT legacy reader #227
Conversation
@kjvbrt should we aim to include this in the tag for the new key4hep release? |
Hi @tmadlener I would be glad If it is included. I'm a bit lost in the CI failures, can you hint what should I fix? |
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.
Are there any tests for this yet? That would make the small refactoring I propose below a bit easier to do, but if you don't have time to fix this now, I think we can also merge this as is and open an issue to keep track of that and address that ASAP after the tag, as the functionality doesn't change.
Also the failing CI workflow seems to have been ignored in the past and is most likely due to a newer version of HepMC in the stack that is used there. |
I will prepare test to check for all the collections in the current yaml file. |
I'm not sure what to do with Podio user data collections. For now they are ignored in the test. |
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.
Nice, thanks for the update.
I think for the user data collections we could cook something up with std::apply
over the podio::SupportedUserDataTypes
which would make it automatically correct, but I would postpone that to another PR as that might require a bit of refactoring around these and I think the better direction would actually be to add an abstract to_json
to podio::CollectionBase
which would get rid of the whole if
-else
chain entirely.
BEGINRELEASENOTES
ENDRELEASENOTES