Skip to content

[skip-ci][rfile] Add RFile python doc - #23633

Open
silverweed wants to merge 1 commit into
root-project:masterfrom
silverweed:rfile_doc_py
Open

silverweed wants to merge 1 commit into
root-project:masterfrom
silverweed:rfile_doc_py

Conversation

@silverweed

Copy link
Copy Markdown
Contributor

No description provided.

@vepadulano vepadulano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot! A few comments from my side.

Comment thread bindings/pyroot/pythonizations/python/ROOT/_pythonization/_rfile.py Outdated
Comment thread bindings/pyroot/pythonizations/python/ROOT/_pythonization/_rfile.py Outdated
Comment thread bindings/pyroot/pythonizations/python/ROOT/_pythonization/_rfile.py Outdated
\code{.py}
# Reading an object from file. Note that Get will return None if no object is stored under the given path.
with ROOT.Experimental.RFile.Open("myfile.root") as file:
myHisto = file.Get("myHisto")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a side note, other Python interface e.g. the dictionary have a get method which raises error by default and optionally returns a default argument which could be None e.g. get("mykey", None).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is currently the behavior on the C++ side; if we strongly believe we should change it we should change it for both languages, but I'd avoid having it behave differently. We still have a bit of time to decide before moving RFile out of Experimental.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wouldn't object keeping things as they are, as long as they're properly documented. If you want to have this discussion with other ROOT team members let me know.

Comment thread bindings/pyroot/pythonizations/python/ROOT/_pythonization/_rfile.py
Comment thread bindings/pyroot/pythonizations/python/ROOT/_pythonization/_rfile.py Outdated
Comment thread bindings/pyroot/pythonizations/python/ROOT/_pythonization/_rfile.py Outdated

@jblomer jblomer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In principal looks good to me. Should we mention Tree/RNTuple with RFile in this doc?

@silverweed

Copy link
Copy Markdown
Contributor Author

Should we mention Tree/RNTuple with RFile in this doc?

Yes, but for RNTuple we should probably wait that at least RNTupleWriter_Append is moved out of experimental so that it becomes a proper overload of RNTupleWriter::Append (happening here).

For TTree we can mention that it may be read but not written.

For both we probably want to redirect to the respective tutorials, which is why I'd wait this PR to be merged before doing so.

@vepadulano vepadulano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you, this was definitely important to address!

\code{.py}
# Reading an object from file. Note that Get will return None if no object is stored under the given path.
with ROOT.Experimental.RFile.Open("myfile.root") as file:
myHisto = file.Get("myHisto")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wouldn't object keeping things as they are, as long as they're properly documented. If you want to have this discussion with other ROOT team members let me know.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants