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

Add LCIO to EDM4hep conversion functionality #11

Merged
merged 87 commits into from
Jun 13, 2023

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Feb 22, 2023

BEGINRELEASENOTES

  • Add LCIO to EDM4hep conversion functionality with a similar interface as the one that is already present for the other direction.

ENDRELEASENOTES

This is the start of a bigger move of existing functionality that exists in another repository to this place. One of the main questions that should be discussed before continuing adding onto this PR is how we want to chunk this up into different PRs, or whether we want to have this go into a non-main branch first and fix things there and then merge into master once we are done with the complete migration.

@jmcarcell
Copy link
Contributor

I think I lean towards a single PR single it's a well defined functionality right? So we can do all the testing at once. But if you think it's too big then splitting in several PRs is fine. Feel free to ping me or ask me for a review when any of the parts is ready for merging.
One comment that I would make is that then the name of the repo doesn't really correspond to what's going to be here so either we choose a new name for the repo k4EDM4hepConverters (since we have converters in either direction, and if there are more in the future they could also go here) or something like that or choose a new place.

@tmadlener
Copy link
Contributor Author

I tend to agree that a single PR would make sense, since it is in principle one feature that we are adding (even if it is a relatively large one). We can also see how large this one would become, and split into smaller chunks for review. I will keep adding to this one then for now.

In principle the interface is ready for review. This includes function naming and argument and return types. Ideally both directions (EDM4hep -> LCIO and LCIO -> EDM4hep) should be as similar as possible. I think we at least started with that goal, but we haven't always checked that along the way, so maybe a fresh look would be able to spot some of the differences that we have managed to sneak in.

As for the repository name, you are right, it is very specific now. However, changing it would probably be a slightly larger effort because it would again touch a few things in the stack, including the name of a spack package, so for now I would leave it as it is, to not complicate this whole endeavour even more.

@tmadlener
Copy link
Contributor Author

Just to give slightly more context of the things that should still be expected in this PR in order to facilitate reviewing and to try and give the "big picture".

  • An executable linking against the edmCompare library in tests that takes an LCIO and an EDM4hep file as inputs and compares their contents. We used this for testing the comparison during development. We have to rework this first to make use of the Frame instead of the EventStore, but that should be rather straight forward
  • A standalone executable for converting an LCIO input file to an EDM4hep file. Potentially also one in the other direction, but there seems to not be too much demand for that at the moment. This could potentially be part of a separate PR, as it could be considered a separate feature.

One thing that could also be done with the edmCompare library is to use it in more fine grained unit tests, but we haven't started any work in that direction yet. Probably would be part of a separate PR.

@jmcarcell jmcarcell self-requested a review March 1, 2023 12:13
@jmcarcell
Copy link
Contributor

I updated the branch with the recent changes that rename TPCHit to RawTimeSeries and I can build with the most recent versions of the packages

@tmadlener
Copy link
Contributor Author

@jmcarcell the crashes you reported today in the meeting should be fixed with the latest fixes from Finn.

We will try to address the missing CaloHitContributions next and also think about a test strategy that can be easily put into CI.

@hegner
Copy link
Contributor

hegner commented May 25, 2023

@tmadlener - I added a metadata todo to the initial ticket

@tmadlener tmadlener changed the title [WIP] Add LCIO to EDM4hep conversion functionality Add LCIO to EDM4hep conversion functionality Jun 5, 2023
@tmadlener
Copy link
Contributor Author

As discussed in todays meeting; We have fairly extensively tested this and it covers (at least) the same as the old converter (and some additional things). I will merge this in order to get it to run in "production", where we will likely see a few more issues in some of the details. I will also open issues to keep track of the open TODO points in this PR.

@tmadlener tmadlener merged commit b50bef5 into key4hep:master Jun 13, 2023
@tmadlener tmadlener deleted the add-lcioconv branch June 27, 2023 09:15
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.

4 participants