tensor slicing logic for negative steps #7410
Conversation
|
Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes. |
michal-bakshi
changed the title
There was a problem hiding this comment.
Pull request overview
This PR implements support for negative step values in tensor slicing operations, enabling reverse traversal similar to Python/NumPy behavior. Previously, the Slice function only supported forward traversal with positive steps.
Changes:
- Removed the restriction that prevented negative step values in
Tensor::Slice - Added logic to handle negative steps with proper index wrapping, boundary checks, and special handling for
stop=-1(meaning "before index 0") - Updated default slice parameters to correctly initialize based on step direction (forward vs reverse)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cpp/open3d/core/Tensor.cpp | Implements negative step support with proper boundary handling and stride calculation |
| cpp/open3d/core/TensorKey.cpp | Updates default slice parameter initialization based on step direction |
| cpp/tests/core/Tensor.cpp | Adds comprehensive test cases for negative step slicing scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cpp/open3d/core/Tensor.cpp
Outdated
| SizeVector new_shape = shape_; | ||
| SizeVector new_strides = strides_; | ||
| new_shape[dim] = (stop - start + step - 1) / step; | ||
| new_shape[dim] = (start - stop + abs_step - 1) / abs_step; |
There was a problem hiding this comment.
For negative steps, when stop == -1 (meaning before index 0), this calculation may produce incorrect results. The formula (start - stop + abs_step - 1) / abs_step becomes (start - (-1) + abs_step - 1) / abs_step = (start + abs_step) / abs_step, which doesn't correctly compute the number of elements from start down to (but not including) index 0. The correct calculation should be (start + 1) / abs_step when stop == -1, or more generally (start - stop) / abs_step since stop is already adjusted.
There was a problem hiding this comment.
I believe the current formula already handles the stop == -1 case correctly.
Below is the reasoning.
Logical Distance: When stop == -1 with a negative step, we intend to include index 0. The span is therefore start - (-1) = start + 1. This correctly represents the total pool of elements.
Element Counting Logic: The formula (start - stop + abs_step - 1) / abs_step ensures that any element touched by a step is counted. When stop == -1, this simplifies to (start + abs_step) / abs_step, which can be logically viewed as 1 + (start / abs_step). The 1 accounts for the initial element at start, and start / abs_step accounts for subsequent elements reachable within the distance to 0.
Validation against suggested formulas: Using (start + 1) / abs_step would fail whenever the distance is not perfectly divisible by the step. For example, with start = 2 and step = -2, the correct elements are {2, 0} (count = 2).
My formula: (2 - (-1) + 2 - 1) / 2 = 2 (Correct)
Suggested formula: (2 + 1) / 2 = 1 (Incorrect, misses index 0).
|
Hi @michal-bakshi thanks for submitting this nice feature! Can you please check the CI errors? |
|
Hi @ssheorey |
2 participants
Type
Motivation and Context
This PR implements support for negative step values in the
Tensor::Slicefunction, addressing a TODO item for reverse traversal slicing operations.Checklist:
python util/check_style.py --applyto apply Open3D code styleto my code.
updated accordingly.
results (e.g. screenshots or numbers) here.
Description
Problem:
Previously, the
Slicefunction only supported positive step values, which limited tensor slicing to forward traversal only.Solution:
Slicefunctionstop=-1case which means "before index 0" in Python semanticsclose #7409