Skip to content

Refactor retrieval of EDM4hep event Frame into method#238

Merged
jmcarcell merged 4 commits intokey4hep:mainfrom
tmadlener:refactor-edmevent
Aug 11, 2025
Merged

Refactor retrieval of EDM4hep event Frame into method#238
jmcarcell merged 4 commits intokey4hep:mainfrom
tmadlener:refactor-edmevent

Conversation

@tmadlener
Copy link
Member

@tmadlener tmadlener commented May 22, 2025

BEGINRELEASENOTES

  • Refactor the retrieval of the EDM4hep event (Frame) into standalone method

ENDRELEASENOTES

Another standalone outcome of #236

As far as I can tell there is no way that the optional is not engaged that didn't already break before. The behavior before was

  • If there is a PodioDataSvc, we get the event Frame from there
  • Otherwise we get the Frame from where the IOSvc puts it
    • In case it is not there we create an empty local Frame

@jmcarcell anything I missed here about the previous behavior?

@tmadlener
Copy link
Member Author

tmadlener commented May 22, 2025

Playing around with godbolt, it indeed looks like the previous version only worked by accident (if it worked at all) (reproducer).

I have switched to returning a const & and throwing an exception in case there is no valid Frame available. At least for our tests everything seems to work.

@tmadlener
Copy link
Member Author

Tests are passing for me locally. I have to investigate why they are failing here.

@tmadlener
Copy link
Member Author

Not sure why tests were passing for me locally, I can't seem to reproduce this. I have added the necessary functionality to put an empty Frame into the TES if none is there already.

Copy link
Member

@jmcarcell jmcarcell left a comment

Choose a reason for hiding this comment

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

Looks good. I edited the details about removing std::optional and std::reference_wrapper, which now is done and was a bit confusing. Also the godbolt link now points to different code 😃

@jmcarcell jmcarcell merged commit 144e6f3 into key4hep:main Aug 11, 2025
7 checks passed
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