Skip to content

fix: correct errors found while testing RNTuple reading and writing - #1828

Merged
ariostas merged 3 commits into
scikit-hep:mainfrom
ariostas:fix/rntuple
Nov 10, 2026
Merged

ariostas merged 3 commits into
scikit-hep:mainfrom
ariostas:fix/rntuple

Conversation

@ariostas

@ariostas ariostas commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

🤖 AI text below 🤖

Part of the split of #1814 (the list is in #1834). These bugs were found while writing the coverage tests of #1814. tests/test_1828_fix_rntuple.py and tests-cuda/test_1828_cov_gpu_rntuple.py hold their regression tests, and every CPU test in them fails on main (for parametrized tests, at least one of their cases).

  • Narrow representations: columns of a narrower representation than the field's result (zigzag, delta, truncated and quantized floats) are decoded in their own dtype and then converted by value.
  • GPU (interpreter="gpu")
    • Compression is decided per page, so a cluster can mix compressed and uncompressed pages.
    • An algorithm or level of 0 means the pages are uncompressed.
    • Bit columns with several pages are placed correctly.
    • Only compressed pages are decompressed.
    • Narrow representations are converted by value.
  • Errors
    • Split columns of a width other than 16, 32 or 64 bits raise a ValueError instead of reading garbage.
    • Unknown field roles raise NotImplementedError instead of an AssertionError.
    • An anonymous field's index raises a ValueError.
  • Fields and metadata
    • Field ancestry is compared by identity.
    • iterate passes ak_add_doc.
    • MetaData raises AttributeError for missing attributes, so hasattr and pickling work.
    • The name of a linked attribute set no longer overwrites the metadata's own name.
  • Writing
    • The anchor key is big when it lands past 2 GB.
    • Masked arrays are packed without entries for the missing values.
    • Fields of unknown type (empty arrays) raise a TypeError that says how to set a type.
    • NTuple_Header has a working repr.

@github-actions github-actions Bot added the type/fix PR title type: fix (set automatically) label Nov 6, 2026
@ariostas
ariostas marked this pull request as ready for review November 6, 2026 14:47
@codecov

codecov Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.96%. Comparing base (7db4af6) to head (05562ad).

Additional details and impacted files
Files with missing lines Coverage Δ
src/uproot/behaviors/RNTuple.py 83.09% <100.00%> (+4.70%) ⬆️
src/uproot/models/RNTuple.py 93.89% <100.00%> (+2.95%) ⬆️
src/uproot/writing/_cascadentuple.py 93.99% <100.00%> (+2.12%) ⬆️

... and 3 files with indirect coverage changes

ariostas added a commit to ariostas/uproot5 that referenced this pull request Nov 6, 2026
Six tests (of Future.result timeouts, ThreadPoolExecutor.shutdown, the order
of LRUCache, and Library equality) were deleted here in favor of tests in
scikit-hep#1829, scikit-hep#1830 and scikit-hep#1832, which are based on this PR; until those are merged
nothing else would cover them. They are now deleted together with the tests
that replace them. A comment no longer points to a file of scikit-hep#1828.

Assisted-by: Claude Code:claude-opus-5-5
ariostas added a commit to ariostas/uproot5 that referenced this pull request Nov 6, 2026
The ROOT fixture also declared classes and writers that only the tests of
scikit-hep#1828 use; if any of them didn't compile with some ROOT version, the tests
here would fail too. gErrorIgnoreLevel is now restored if something fails
while it's lowered. The four LRUCache tests of test_1661 that
test_1832_cov_api_cache.py replaces are deleted here.

Assisted-by: Claude Code:claude-opus-5-5
ianna pushed a commit that referenced this pull request Nov 7, 2026
* test: tighten the existing tests and make their fixtures robust

Changes to existing tests from #1814 that don't depend on its fixes: they
check values instead of only running, restore checks that had been dropped,
close the files they open, and run concurrently (each XRootD server has its
own admin path and waits until it accepts connections, and reset_classes puts
the original model classes back).

Assisted-by: Claude Code:claude-opus-5-5

* test: keep the tests that the new coverage tests replace

Six tests (of Future.result timeouts, ThreadPoolExecutor.shutdown, the order
of LRUCache, and Library equality) were deleted here in favor of tests in
#1829, #1830 and #1832, which are based on this PR; until those are merged
nothing else would cover them. They are now deleted together with the tests
that replace them. A comment no longer points to a file of #1828.

Assisted-by: Claude Code:claude-opus-5-5

* test: iterate over XRootD in one step per file

MET_pt is in one TBasket, so with steps of 15 entries the later steps of a
file had nothing to request, and the XRootD source then sent an empty vector
read, which failed now and then. #1825 stops sending it; until then, these
tests iterate in one step per file.

Assisted-by: Claude Code:claude-opus-5-5

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

@ariostas -- Thanks! LGTM.

Comment thread src/uproot/behaviors/RNTuple.py Resolved
Comment thread src/uproot/models/RNTuple.py Resolved
These were found by the tests of scikit-hep#1814, which are the regression tests here.
The fixes are to columns of narrower representations (zigzag, delta and
truncated or quantized floats), GPU reads of uncompressed and multi-page
columns, split columns of unsupported widths, unknown field roles, field
paths, linked attribute sets, anchors past 2 GB, and writing empty and
masked arrays.

Assisted-by: Claude Code:claude-opus-5-5
The ROOT fixture also declared writers that only the tests of scikit-hep#1832 use, and
if any of them didn't compile with some ROOT version, the tests here would
fail too. The fixture also restores gErrorIgnoreLevel if a declaration fails.

Assisted-by: Claude Code:claude-opus-5-5
Each page now decides whether it is compressed, and uncompressed pages
are recorded as None, so nothing reads the cluster-wide flag anymore.

Assisted-by: Claude Code:claude-opus-5-5
@ariostas
ariostas merged commit 07b8d4a into scikit-hep:main Nov 10, 2026
21 checks passed
ariostas added a commit to ariostas/uproot5 that referenced this pull request Nov 12, 2026
The ROOT fixture also declared classes and writers that only the tests of
scikit-hep#1828 use; if any of them didn't compile with some ROOT version, the tests
here would fail too. gErrorIgnoreLevel is now restored if something fails
while it's lowered. The four LRUCache tests of test_1661 that
test_1832_cov_api_cache.py replaces are deleted here.

Assisted-by: Claude Code:claude-opus-5-5
ariostas added a commit to ariostas/uproot5 that referenced this pull request Nov 12, 2026
The ROOT fixture also declared classes and writers that only the tests of
scikit-hep#1828 use; if any of them didn't compile with some ROOT version, the tests
here would fail too. gErrorIgnoreLevel is now restored if something fails
while it's lowered. The four LRUCache tests of test_1661 that
test_1832_cov_api_cache.py replaces are deleted here.

Assisted-by: Claude Code:claude-opus-5-5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/fix PR title type: fix (set automatically)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants