fix: RNTuple ug-fixing offset array concatenation, adding filter_name

[giedrius2020]

Changes made in RNTuple model:

• Bug-fix: when reading arrays from more than one cluster, data was not identical to TTree. The reason for this was improper array concatenation in case of offset values. More information in here: https://gist.github.com/giedrius2020/c1ca87784779418ee3e938aaca31ff75
• Added a function to filter wanted key values in def keys()
• Minimal change: changed existing "filter_names" variables to "filter_name", because that is how the variable was called in TTree model.

[ariostas]

Thank you for catching this @giedrius2020! I think the issue could be solved in a simpler way by adjusting some of the logic in here. I'll think back at what my reasoning was for this piece.

uproot5/src/uproot/models/RNTuple.py

Lines 511 to 521 in 012df94

	cumsum = 0
	for page_desc in pagelist:
	n_elements = page_desc.num_elements
	tracker_end = tracker + n_elements
	self.read_pagedesc(
	res[tracker:tracker_end], page_desc, dtype_str, dtype, nbits, split
	)
	if delta:
	res[tracker] -= cumsum
	cumsum += numpy.sum(res[tracker:tracker_end])
	tracker = tracker_end

[ariostas]

Could you try checking if changing the last few lines to

if delta:
    if tracker > 0:
        res[tracker] -= cumsum
    cumsum += numpy.sum(res[tracker:tracker_end])
tracker = tracker_end 

fixes the issue?

[davidlange6]

perhaps best that we separate the api change to keys() from the bug fix?

otherwise, perhaps beyond this PR, is there some test in uproot intended to check the physics correctness of reading back files?

[giedrius2020]

Could you try checking if changing the last few lines to

if delta:
    if tracker > 0:
        res[tracker] -= cumsum
    cumsum += numpy.sum(res[tracker:tracker_end])
tracker = tracker_end 

fixes the issue?

@ariostas , I tried your suggestion (while disabling my changes) and it did not help.

The result is still the same, arrays for all cluster look like this:

Offset arrays before adjusting: 
Cluster 1: [    0     0     1 ... 22134 22134 22135]
Cluster 2: [    0     0     2 ... 35045 35046 35048]
Cluster 3: [    0     2     4 ... 34930 34931 34931]
...

While it should be like this:

Offset arrays after adjusting: 
Cluster 1: [    0     0     1 ... 22134 22134 22135]
Cluster 2: [22135 22137 22138 ... 57180 57181 57183]
Cluster 3: [57185 57187 57187 ... 92113 92114 92114]
...

[ariostas]

I tried your suggestion (while disabling my changes) and it did not help.

Ah okay, thanks

perhaps best that we separate the api change to keys() from the bug fix?

Since the RNTuple stuff isn't stable yet, I wouldn't worry much about separating things

otherwise, perhaps beyond this PR, is there some test in uproot intended to check the physics correctness of reading back files?

I've added some tests, but the issue was that I didn't have a test file that was large enough to have multiple clusters, so that's why I hadn't seen this bug

[davidlange6]

can one control cluster size in rntuple writing? indeed, i guess one doesn't want a few 100 MB of test file around

[ariostas]

can one control cluster size in rntuple writing?

I'm not sure, but that would be nice. I'll look into it

[ariostas]

Thank you @giedrius2020, this looks great. The test that is failing is unrelated.

I'm going to try to generate a small RNTuple with multiple clusters so that we can add a test for this.

[ariostas]

I added a new test file in scikit-hep/scikit-hep-testdata#159. Let's add a new test once that gets merged and released.

[jpivarski]

That worked! I'm updating again to be sure that we get all of the 3.13 tests.

[jpivarski]

Now this should include the pyodide-build, and I'll enable auto-merge because I'm sure it will pass.

[jpivarski]

@all-contributors please add @giedrius2020 for code

[allcontributors]

@jpivarski

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