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

Use podio::Reader and podio::Writer with IOSvc #233

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

jmcarcell
Copy link
Contributor

@jmcarcell jmcarcell commented Sep 6, 2024

BEGINRELEASENOTES

  • Use podio::Reader and podio::Writer with IOSvc, so that it is easy to write TTrees or RNTuples by changing the IOType parameter given to IOSvc in the steering file.

ENDRELEASENOTES

Copy link
Contributor

@m-fila m-fila left a comment

Choose a reason for hiding this comment

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

Is there a reason to dynamically allocate the writer and reader?

  • They already are type erased
  • getWriter() always return a valid object and is always assumed to do so
  • I think the wrtier's finish method could be called directly instead triggering with pointer reset, no?

Comment on lines +46 to +47
if (m_outputType != "default" && m_outputType != "ROOT" && m_outputType != "RNTuple") {
error() << "Unknown input type: " << m_outputType << ", expected ROOT, RNTuple or default" << endmsg;
Copy link
Contributor

Choose a reason for hiding this comment

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

podio::Writer allows also SIO. Is this intentional to exclude it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The demand for writing EDM4hep files using SIO is quite low. If someone wants it we can add it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be nice for some testing at some point, but I agree we can easily toggle this once it becomes necessary.

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.

3 participants