Skip to content

[cppyy] Set LowLevelView itemsize to sizeof(T) for fixed multidim arrays#21388

Merged
aaronj0 merged 3 commits intoroot-project:masterfrom
aaronj0:fix-lowlevel-fixed-itemsize
Feb 27, 2026
Merged

[cppyy] Set LowLevelView itemsize to sizeof(T) for fixed multidim arrays#21388
aaronj0 merged 3 commits intoroot-project:masterfrom
aaronj0:fix-lowlevel-fixed-itemsize

Conversation

@aaronj0
Copy link
Copy Markdown
Contributor

@aaronj0 aaronj0 commented Feb 26, 2026

For fixed contiguous C arrays (e.g. float a[2][2]), the ndim > 1 path in CreateLowLevelViewT was unconditionally setting view.itemsize to sizeof(void*), which is correct for pointer-to-pointer arrays but does not work for fixed arrays. Fixes #21378

For fixed contiguous C arrays (e.g. float a[2][2]), the ndim > 1 path in CreateLowLevelViewT was unconditionally setting view.itemsize to sizeof(void*), which is correct for pointer-to-pointer arrays but does not work for fixed arrays. Fixes root-project#21378
Copy link
Copy Markdown
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

This is great! Could you also add a test?

@aaronj0
Copy link
Copy Markdown
Contributor Author

aaronj0 commented Feb 26, 2026

This is great! Could you also add a test?

neat, this actually revealed a bug in a test pyunittests-bindings-pyroot-pythonizations-pyroot-array-numpy-views that I wrote for fixed-width integer low level views: #18492 :)

Back then, I didn't realise why I had to read the buffer as 1D and reshape the resulting Py_buffer but now it all makes sense...
In the test, we see the same case, view.len = 16, and view.itemsize = 8 so frombuffer saw 2 elements of size 8, but count=22 forced it to reinterpret the raw bytes as 22 elements anyway. That's a bad pattern..

With this patch, frombuffer correctly sees 4 floats, but count=22 now exceeds the buffer size (as it should). Now that the underlying issue is fixed, we can just use np.array(cpp_arr, dtype=dtype[0]) and it works as expected!

…numpy

This was a (terrible) workaround to the fact that resulting views of multidim arrays did not convey the right itemsize based on the underlying type. Now that root-project#21378 is fixed, the workaround breaks and we can just use `np.array(arr, dtype)` which is a lot better and Pythonic for users.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 26, 2026

Test Results

    22 files      22 suites   3d 2h 31m 27s ⏱️
 3 807 tests  3 804 ✅ 1 💤 2 ❌
75 691 runs  75 680 ✅ 9 💤 2 ❌

For more details on these failures, see this check.

Results for commit 22b34ec.

♻️ This comment has been updated with latest results.

@aaronj0 aaronj0 force-pushed the fix-lowlevel-fixed-itemsize branch from cbf159a to 6d9f885 Compare February 26, 2026 21:29
Copy link
Copy Markdown
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

Thank you!

@aaronj0 aaronj0 force-pushed the fix-lowlevel-fixed-itemsize branch from 6d9f885 to 22b34ec Compare February 27, 2026 09:32
@aaronj0
Copy link
Copy Markdown
Contributor Author

aaronj0 commented Feb 27, 2026

ruff formatting has been addressed
edit: it now complains about the rest of the file, and not the added test itself

@vepadulano
Copy link
Copy Markdown
Member

ruff formatting has been addressed
edit: it now complains about the rest of the file, and not the added test itself

Yep, that's expected. You can choose to either merge this PR as is, or take the chance to ruff format and ruff check the entire file, but then those changes would go in a separate commit. Up to you.

@aaronj0
Copy link
Copy Markdown
Contributor Author

aaronj0 commented Feb 27, 2026

ruff formatting has been addressed
edit: it now complains about the rest of the file, and not the added test itself

Yep, that's expected. You can choose to either merge this PR as is, or take the chance to ruff format and ruff check the entire file, but then those changes would go in a separate commit. Up to you.

As we want to minimize the diff with upstream, we should keep the cppyy/test files the same.

Merging, as test failures on mac-beta (h2root) and ubuntu26 (python-distrdf-backends-test_all) can be seen on master

@aaronj0 aaronj0 merged commit 2a0a8e5 into root-project:master Feb 27, 2026
25 of 30 checks passed
@aaronj0
Copy link
Copy Markdown
Contributor Author

aaronj0 commented Feb 27, 2026

/backport to 6.38

@root-project-bot
Copy link
Copy Markdown

Something went wrong with the backport to 6.38: @aaronj0 please see the logs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Python] NumPy conversion fails for multidimensional arrays (incorrect itemsize in LowLevelView)

3 participants