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

Indexing order for 3D integer_array_t doesn't make sense #1011

Open
bradleyharden opened this issue Apr 30, 2024 · 0 comments
Open

Indexing order for 3D integer_array_t doesn't make sense #1011

bradleyharden opened this issue Apr 30, 2024 · 0 comments

Comments

@bradleyharden
Copy link
Contributor

bradleyharden commented Apr 30, 2024

The indexing order for the 3D version of integer_array_t doesn't make any sense. You can see the code for get here:

return get(arr.data, (y*arr.width + x)*arr.depth + z);

y is multiplied by both width and depth, x is only multiplied by depth, and z isn't multiplied by anything.

The indexing of get is consistent with set, which is why it hasn't been caught until now. The problem comes when you try to index after a reshape, which is how I discovered the problem. If you reshape a 3D array into a 2D array, you can't get results that make sense.

The "obvious" solution would be

< return get(arr.data, (y*arr.width + x)*arr.depth + z);
---
> return get(arr.data, (x*arr.width + y)*arr.depth + z);

But this solution would end up inconsistent with the 2D version of get:

return get(arr.data, y*arr.width + x);

I'm not really sure what to do here. It seems like the change that would be least likely to break existing code would be

return get(arr.data, (z*arr.depth + y)*arr.width + x);

since that is consistent with the existing indexing order for 2D arrays.

Has anyone ever decided whether integer_array_t stores data in row-major or column-major order? The indexing for 2D arrays suggests column-major, but that goes against C and Python. It seems like that is something that should be documented and guaranteed.

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

No branches or pull requests

1 participant