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

chore: update reading support for RNTuple v1 #1338

Merged
merged 9 commits into from
Nov 19, 2024
Merged

Conversation

ariostas
Copy link
Collaborator

This PR adds support for the first stable version of the RNTuple binary format (v1.0.0.0). It's mostly just rearranging some things and adjusting some constants. Tests will fail until scikit-hep/scikit-hep-testdata#164 is merged and released. This is still a work in progress, but it seems to mostly work already.

Note that this will completely break support for any files produced before v1

@ariostas
Copy link
Collaborator Author

This is now ready for review. All the previous functionality works again, and I fixed a small bug. All the tests should pass after scikit-hep/scikit-hep-testdata#164 is merged and released.

The remaining things that are not supported are:

  • Real32Trunc and Real32Quant columns
  • Suppressed columns
  • Objects serialized with the ROOT streamer (which will probably never work)

Instead of waiting until I finish implementing the first two, I think it would be good to get this out so that people can try it out and we can get some feedback.

@ariostas ariostas marked this pull request as ready for review November 18, 2024 20:41
@ariostas ariostas changed the title chore: update support for RNTuple v1 chore: update reading support for RNTuple v1 Nov 19, 2024
@ariostas ariostas requested a review from jpivarski November 19, 2024 15:54
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

This seems to be a major overhaul to adapt for RNTuple 1.0.0.0, and it looks good!

Previously, you disabled the RNTuple-based tests. Ah! Okay, I see where the pytest.skip calls were removed. (I was looking for them as decorators, but they were module-level.)

So with the tests reinstated and new files in scikit-hep-testdata, everything works—I think it's ready to be merged! Thanks!

@ariostas ariostas merged commit 2a72f67 into main Nov 19, 2024
27 checks passed
@ariostas ariostas deleted the ariostas/rntuple_v1 branch November 19, 2024 20:00
@jpivarski
Copy link
Member

@all-contributors please add @ariostas for code

Copy link
Contributor

@jpivarski

I've put up a pull request to add @ariostas! 🎉

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.

2 participants