Unsure about correctness of the implementation of IsContiguous
- 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.