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

edm4hep2json: All EDM4hep collections + ROOT legacy reader #227

Merged
merged 9 commits into from
Nov 1, 2023

Conversation

kjvbrt
Copy link
Contributor

@kjvbrt kjvbrt commented Sep 20, 2023

BEGINRELEASENOTES

  • edm4hep2json now converts all EDM4hep collections, associations and Podio user data
  • added ROOT legacy support for edm4hep2json

ENDRELEASENOTES

@kjvbrt kjvbrt mentioned this pull request Sep 21, 2023
@tmadlener
Copy link
Contributor

@kjvbrt should we aim to include this in the tag for the new key4hep release?

@kjvbrt
Copy link
Contributor Author

kjvbrt commented Oct 30, 2023

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?

Copy link
Contributor

@tmadlener tmadlener left a 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.

tools/include/edm4hep2json.hxx Outdated Show resolved Hide resolved
@tmadlener
Copy link
Contributor

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.

@kjvbrt
Copy link
Contributor Author

kjvbrt commented Nov 1, 2023

I will prepare test to check for all the collections in the current yaml file.

test/tools/test_all_collections.py Outdated Show resolved Hide resolved
test/tools/test_all_collections.py Outdated Show resolved Hide resolved
test/tools/test_all_collections.py Outdated Show resolved Hide resolved
@kjvbrt
Copy link
Contributor Author

kjvbrt commented Nov 1, 2023

I'm not sure what to do with Podio user data collections. For now they are ignored in the test.

Copy link
Contributor

@tmadlener tmadlener left a 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.

@tmadlener tmadlener merged commit eba6e61 into key4hep:main Nov 1, 2023
7 of 9 checks passed
@kjvbrt kjvbrt deleted the all-coll branch November 2, 2023 07:51
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.

2 participants