Skip to content

feat: RNTuple reading improvements#1406

Merged
ianna merged 17 commits intomainfrom
ariostas/rntuple_reading_improvements
Apr 1, 2025
Merged

feat: RNTuple reading improvements#1406
ianna merged 17 commits intomainfrom
ariostas/rntuple_reading_improvements

Conversation

@ariostas
Copy link
Copy Markdown
Member

This is still a work in progress. I'm adding more functionality for RNTuple reading so that methods are on par with what is available for TTrees. In particular, I'm adding missing functionality to keys, arrays, and similar methods. There's also other minor tweaks and fixes, and a lot more documentation.

I introduced a new HasFields class that plays a similar role for RNTuple and RField, as HasBranches does for TTree and TBranch. I'm basing things on the existing code for TBranch. However, since RNTuple is more "fundamental" than RField, and RField is a "fake" class that is not read directly, I'm structuring things a bit differently.

@ariostas
Copy link
Copy Markdown
Member Author

I ended up not adding all the functionality, since I didn't realize how robust those functions are for TTrees. However, I added all the most commonly used functionality, and I'll follow up implementing missing things on future PRs.

There were some significant changes to keys and related methods. Now nothing is hidden from the user, so they have access to the full RNTuple structure, even though some subfields are not really useful. You can also now see the full structure with .show().

I also fixed the tupleness of tuple-like data (std::pair and std::tuple), and I added some more documentation.

@ariostas ariostas marked this pull request as ready for review March 26, 2025 20:44
@ariostas ariostas requested a review from ianna March 26, 2025 20:45
@ariostas
Copy link
Copy Markdown
Member Author

Just out of curiosity, I'll have Copilot review this PR to see how it works.

@ariostas ariostas requested a review from Copilot March 26, 2025 20:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds improved RNTuple reading functionality along with new tests covering methods such as keys, getitem, to_akform, and iteration/concatenation behavior. The changes include adjustments in expected array lengths, updated output formats for empty structures, and refinements in container handling for RNTuple tests.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/test_1406_improved_rntuple_methods.py New tests added for keys, array access, to_akform conversion, and iteration/concatenation functionality.
tests/test_1250_rntuple_improvements.py Updated assertions in field class and iterate tests to reflect new behavior.
tests/test_1223_more_rntuple_types.py Modified expected output for empty structures from dicts to tuples.
tests/test_0662_rntuple_stl_containers.py Adjusted keys call and tuple conversion assertions for STL container tests.
tests/test_0630_rntuple_basics.py Updated keys assertions to use nonrecursive calls for clarity.
Comments suppressed due to low confidence (2)

tests/test_1250_rntuple_improvements.py:23

  • Verify that updating the expected count for 'v' from 0 to 1 reflects the intended behavior for empty structures in RField.
assert len(v) == 1

tests/test_1250_rntuple_improvements.py:58

  • [nitpick] Consider adding a comment to clarify the rationale behind the new magic number (188) for the array length in the iteration tests.
assert len(arrays) == 188

Copy link
Copy Markdown
Member

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@ariostas - Thanks! Looks good to me. The copilot review is quite detailed :-)
I'm merging it.

@ianna ianna merged commit 9c00e1d into main Apr 1, 2025
26 checks passed
@ianna ianna deleted the ariostas/rntuple_reading_improvements branch April 1, 2025 08:34
@ariostas ariostas mentioned this pull request Apr 21, 2025
26 tasks
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