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

Merged
merged 12 commits into from
Oct 1, 2024
Merged

Conversation

jmcarcell
Copy link
Member

@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
Member 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.

@jmcarcell jmcarcell force-pushed the reader-writer branch 2 times, most recently from df4025e to b74dd1b Compare September 22, 2024 16:29
@jmcarcell
Copy link
Member Author

Now the Reader and Writer are stored in an std::optional and there isn't any dynamic allocation.

I think the wrtier's finish method could be called directly instead triggering with pointer reset, no?

We could but then the writer can't be used anymore so we may as well delete it, which prevents further usage of the writer possibly by mistake.

Comment on lines +47 to +48
virtual void deleteWriter() = 0;
virtual void deleteReader() = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be named differently now that they are no longer pointer backed? Something like resetWriter|resetReader?

Copy link
Member Author

@jmcarcell jmcarcell Oct 1, 2024

Choose a reason for hiding this comment

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

I think it doesn't matter but the reason why they are needed is because services in Gaudi are not deleted so making the connection would be easier in case one wants to look for the old issue where this is described in Gaudi and technically they are being deleted.

@jmcarcell jmcarcell merged commit dcca269 into main Oct 1, 2024
4 of 9 checks passed
@jmcarcell jmcarcell deleted the reader-writer branch October 1, 2024 17:59
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