isl-org / isl-org/Open3D

Unsure about correctness of the implementation of IsContiguous

Open
#7,363 1 comment 0 reactions 0 assignees View on GitHub
question
Dominant language
C++
Stars
14k
Forks
2.6k
Avg merge
5d 18h
Merged PRs (30d)
6

Description

### Checklist

- [x] I have searched for [similar issues](https://github.com/isl-org/Open3D/issues).
- [x] For Python issues, I have tested with the [latest development wheel](https://www.open3d.org/docs/latest/getting_started.html#development-version-pip).
- [x] I have checked the [release documentation](https://www.open3d.org/docs/release/) and the [latest documentation](https://www.open3d.org/docs/latest/) (for `main` branch).

### My Question

The definition of IsContiguous is not consistent with its meaning.
The current implementation is equivalent to an imaginary function IsDefaultStrided.
I would expect a contiguous tensor is one which does not "skip over" bytes in its buffer. While it is definitely true that a non-contiguous tensor will definitely be strided in a non-default way, the inverse **is not true**.
**A non-default-strided tensor is not necessarily non-contiguous.**

Example:
```cpp
// imagine a voxel grid of integers sized 2x3x4.
// let's say we have a host array which is y-coordinate-major, meaning that y has the largest stride.
// this can be useful if we want certain views of the data (like ones along the Y axis) to be contiguous.
int valsAtCoords[24] = { val(x_0, y_0, z_0), val(x_0, y_0, z_1), val(x_0, y_0, z_2), ..., val(x_1, y_0, z_0), val(x_1, y_0, z_1), ..., val(x_0, y_1, z_1), ...) };
Tensor a(valsAtCoords, core::Int32, /*shape*/ {2, 3, 4}, /*strides*/ {12, 4, 1});
std::cout << a.IsContiguous(); // True
// now we init a tensor from the same buffer, with the same size, but different stride to reflect the layout of the data in the host buffer
Tensor b(valsAtCoords, core::Int32, /*shape*/ {2, 3, 4}, /*strides*/ {4, 8, 1});
std::cout << b.IsContiguous(); // False! Even though the buffer is contiguous.
```

If I am understanding this correctly, I believe the IsContiguous method is incorrect.
I have noticed that in some usage scenarios in the library, the calling code is expecting it to actually mean IsDefaultStrided (which it does now).
However, this should be corrected. There are many other operations which check IsContiguous purely to verify that buffers are without holes, and if IsContiguous returns false, it's a de-optimization as code needs to follow a different - non-optimal - path.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.