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

fix: only compile edm4eic_merge for podio < 0.17.4 #64

Merged
merged 1 commit into from
Jan 21, 2024

Conversation

wdconinc
Copy link
Contributor

Briefly, what does this PR introduce?

With podio 0.17.4, we can't compile emd4eic_merge anymore since it is reliant on podio/EventStore.h. We have been ignoring the deprecation warnings about this for half a year now...

This PR makes the compilation of edm4eic_merge conditional on being before that version. It renames the source and executable to event_merge.cpp and edm4eic_event_merge to make clear this isn't working for frames. Code is not removed since an adventurous soul may port this to frames so we can do background mixing.

What kind of change does this PR introduce?

  • Bug fix (issue: edm4eic doens't compile with podio 0.17.4)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

Yes, edm4eic_merge is renamed, and will not be installed when newer versions of podio are used.

@wdconinc wdconinc requested a review from veprbl January 19, 2024 21:33
@wdconinc wdconinc enabled auto-merge (squash) January 19, 2024 21:33
@wdconinc wdconinc linked an issue Jan 20, 2024 that may be closed by this pull request
@veprbl
Copy link
Member

veprbl commented Jan 20, 2024

Can we just remove this. I don't see how eventstore logic will help revive this.

@wdconinc
Copy link
Contributor Author

I was thinking about that but we don't have any other podio merging code elsewhere (might be something that should go upstream). At this point we've mostly been focusing background efforts on hepmc3 merging, but the goal was to transition to merging after simulation at some point. This could be something that someone can start from.

So, how about we upstream it as a general podio tool and then remove it here?

@veprbl
Copy link
Member

veprbl commented Jan 20, 2024

I'm not sure why do we need to work on a tool that no one uses. Likely, for merging, hadd should be used. Its output may be slightly broken in metadata, but for that, podio can be fixed.

@wdconinc
Copy link
Contributor Author

hadd merges one event with one event across input files. It doesn't do one to N, and it doesn't allow N to vary across files, and it doesn't allow N to be a Poisson distributed random variable. Even as a starting point, I wouldn't recommend hadd because you would find yourself be limited very quickly.

@wdconinc wdconinc merged commit 773bf26 into main Jan 21, 2024
3 checks passed
@wdconinc wdconinc deleted the edm4eic-merge-no-more branch January 21, 2024 03:32
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.

EventStore used in merge utility is removed in podio 0.17.4
2 participants