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

fix: various RNTuple fixes #1191

Merged
merged 16 commits into from
May 2, 2024
Merged

fix: various RNTuple fixes #1191

merged 16 commits into from
May 2, 2024

Conversation

ariostas
Copy link
Collaborator

@ariostas ariostas commented Apr 1, 2024

Very simple RNTuple files can be read, but currently there are issues reading more complex files. In this PR, I'll be working towards fixing as many issues as I can find. I'll update this description with more information once it's ready for review.

@ariostas ariostas changed the title RNTuple fixes fix: various RNTuple fixes Apr 1, 2024
@ariostas ariostas marked this pull request as ready for review April 24, 2024 20:51
@jpivarski jpivarski self-requested a review May 2, 2024 15:05
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.

Very nice! I've scanned through everything and it's clear and self-explanatory. It looks like some dtypes changed, or maybe they're only getting tested now.

Please merge this to confirm that you're done with the PR. Thanks!

@ariostas ariostas merged commit 501a12f into main May 2, 2024
24 checks passed
@ariostas ariostas deleted the ariostas/fix_rntuple branch May 2, 2024 18:16
@jpivarski
Copy link
Member

FYI, one of the tests in tests/test_1191_rntuple_fixes.py is pretty slow. My test-runner seemed to be stuck indefinitely, so I reran it with --durations=0. It was a lot faster the second time, so it must have been stuck on something the first time, but still, 5 seconds is a bit long.

==================================== slowest durations =====================================
5.58s call     tests/test_1191_rntuple_fixes.py::test_multiple_page_lists
0.21s call     tests/test_1191_rntuple_fixes.py::test_schema_extension
0.11s call     tests/test_1191_rntuple_fixes.py::test_skip_recursively_empty_structs
0.03s call     tests/test_1191_rntuple_fixes.py::test_empty_page_list
0.02s call     tests/test_1191_rntuple_fixes.py::test_rntuple_cardinality
0.00s call     tests/test_1191_rntuple_fixes.py::test_split_encoding
0.00s setup    tests/test_1191_rntuple_fixes.py::test_schema_extension
0.00s teardown tests/test_1191_rntuple_fixes.py::test_multiple_page_lists
0.00s teardown tests/test_1191_rntuple_fixes.py::test_skip_recursively_empty_structs
0.00s teardown tests/test_1191_rntuple_fixes.py::test_schema_extension
0.00s setup    tests/test_1191_rntuple_fixes.py::test_split_encoding
0.00s setup    tests/test_1191_rntuple_fixes.py::test_empty_page_list
0.00s teardown tests/test_1191_rntuple_fixes.py::test_split_encoding
0.00s setup    tests/test_1191_rntuple_fixes.py::test_rntuple_cardinality
0.00s teardown tests/test_1191_rntuple_fixes.py::test_rntuple_cardinality
0.00s teardown tests/test_1191_rntuple_fixes.py::test_empty_page_list
0.00s setup    tests/test_1191_rntuple_fixes.py::test_skip_recursively_empty_structs
0.00s setup    tests/test_1191_rntuple_fixes.py::test_multiple_page_lists

Maybe it was stuck downloading http://root.cern/files/tutorials/ntpl004_dimuon_v1rc2.root.

We have a network pytest mark to label tests that have to access the network, so that we can say

pytest -m "not network" tests

to exclude them on a quick-test run. That would be good to add, in the future.

@ariostas
Copy link
Collaborator Author

ariostas commented May 7, 2024

Ah okay, sorry about that. I'll try to find/generate a replacement file. Thanks @jpivarski

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.

None yet

2 participants