feat(stride_view): add a strided random-access range adaptor#860
feat(stride_view): add a strided random-access range adaptor#860IvanaGyro wants to merge 2 commits into
Conversation
`stride_view<V>` wraps any sized random-access range `V` and presents every
`step`-th element as a fresh random-access range, with a `r | stride(k)` pipe
form so range algorithms can consume strided slices directly. The iterator
stores the underlying base iterator (not a pointer back to the view), which
keeps iterators valid as long as the underlying data lives, independent of the
view's lifetime.
This is the building block the next Trace refactor needs to reduce a tensor
diagonal in place without first cloning the data into a contiguous buffer.
Landing it as its own change makes the iterator semantics reviewable in
isolation and lets the diagonal-sum step focus on the trace algorithm itself.
`tests/linalg_test/stride_view_test.cpp` covers the random-access iterator
concept model (via `static_assert`), strided element selection, the
non-dividing-size branch of `size()`, the `step == 0` precondition, the pipe
adaptor (fed into `PairwiseSum` to exercise the integration point), the
iterator-outlives-view case, the random-access operations, and a complex
strided sum. The test reaches into the internal header through a new
`${CMAKE_SOURCE_DIR}/src` PRIVATE include on the test binary so internal
utilities can be unit-tested directly.
Co-authored-by: Claude <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces stride_view, a view utility over every step-th element of a random-access, sized range, along with its corresponding unit tests. The review feedback highlights two potential issues in the iterator implementation: first, operator[] performs unsigned addition which can cause unexpected behavior when indexing with negative values; second, operator-= is implemented via negation (-n), which can trigger undefined behavior due to signed overflow if n is the minimum value of difference_type. It is recommended to perform these calculations in signed space and implement subtraction directly.
| } | ||
|
|
||
| iterator& operator+=(difference_type n) { | ||
| index_ = static_cast<std::size_t>(static_cast<difference_type>(index_) + n); |
There was a problem hiding this comment.
What if static_cast<difference_type>(index_) + n < 0? How will common lib like stl handle this case? Raise an error?
There was a problem hiding this comment.
With the 1e86067 fix, static_cast<difference_type>(index_) + n < 0 is computed in signed space and produces a negative index_ (after the cast back), which is exactly how the STL's __normal_iterator / pointer iterators behave: stepping before begin() is undefined behavior, not a checked error — no STL container iterator throws here. stride_view::iterator matches that contract (it models random_access_iterator, whose operator[]/+= have a narrow contract and no bounds check). Reaching a negative index is only valid as an intermediate when you step back to a valid position before dereferencing; RandomAccessOps exercises that with mid[-2] and *(end() - 1). Adding a throw would make it non-conforming (and slower in the hot diagonal-sum loop).
Generated by Claude Code
|
|
||
| // Range-adaptor closure so a strided view can be written as | ||
| // `range | stride(step)`, e.g. `std::span<const T>(data, extent) | stride(k)`. | ||
| struct stride_closure { |
There was a problem hiding this comment.
should we put this in views like std::ranges::views::stride() <=> std::ranges::stride_view
There was a problem hiding this comment.
I kept it as cytnx::linalg_internal::stride / stride_view rather than a views:: namespace, for two reasons:
- C++20 has no
std::ranges::views::stride— that's a C++23 addition (std::ranges::stride_view). Mirroring theviews::spelling under our own namespace would invite confusion with the standard one once we move to C++23, where we'd want to just delete this and usestd::views::stride. - This is an internal backend utility (
linalg_internal), not a public range adaptor we're committing to as API. The flatstride()closure factory is enough for the one consumer (Trace).
If/when the project moves to C++23 this whole header can be replaced by std::views::stride; I left the pipe form r | stride(k) deliberately matching that future signature so the call sites won't change. Happy to add a // TODO(C++23): replace with std::views::stride if you'd like that recorded in the source.
Generated by Claude Code
| stride_view sv(std::span<const double>(v), 2); // 0,2,4,...,18 | ||
| auto it = sv.begin(); | ||
| EXPECT_DOUBLE_EQ(*(it + 3), 6.0); | ||
| EXPECT_DOUBLE_EQ(it[4], 8.0); |
There was a problem hiding this comment.
test random access with negative indices/diff.
There was a problem hiding this comment.
Done in 1e86067. RandomAccessOps now exercises negative offsets: auto mid = sv.begin() + 5; EXPECT_DOUBLE_EQ(mid[-2], 6.0); (negative subscript) and EXPECT_DOUBLE_EQ(*(sv.end() - 1), 18.0) (backward step from end()). These are exactly the paths the signed-space operator[] / operator-= fixes make well-defined.
Generated by Claude Code
IvanaGyro
left a comment
IvanaGyro
left a comment
There was a problem hiding this comment.
(Review written by Claude on behalf of @IvanaGyro.)
Clean adaptor and it resolves both concerns from the earlier round: the test file is now wired into tests/CMakeLists.txt (with src/ exposed for the internal header), and the iterator stores the base iterator rather than a back-pointer to the view, with IteratorsOutliveTheView locking that in. The static_asserts on the iterator/range concepts plus the pipe-adaptor + PairwiseSum integration tests are exactly the right shape. One small edge-case test gap noted inline.
…tests stride_view::iterator::operator-= computed `*this += -n`, which negates the signed difference_type argument. For n == std::numeric_limits<difference_type>::min() the negation overflows -- undefined behavior. Subtract the offset directly so no value of n is negated. Reorder declarations so each class lists its public interface before its data members, matching the Google C++ style guide's declaration order. Document the view with doxygen-style block comments: class purpose with a short usage example, constructor preconditions, size() contract on empty and sub-stride ranges, and the stride() / stride_closure pipe adaptor. Expand the iterator and view tests to cover boundaries reviewers flagged: * SizeRoundsUpWhenLengthNotMultipleOfStride: a 10-element range with stride 3 yields 4 elements (indices 0, 3, 6, 9), pinning the (n + step - 1) / step rounding in size() and end() that the existing exact-multiple case does not reach. * EmptyBaseYieldsEmptyView: size()==0, begin()==end(), and PairwiseSum over the empty piped view returns the identity. * StrideLargerThanBaseSelectsFirstElement: a 3-element range with stride 10 yields a single-element view containing the first element. * RandomAccessOps: a negative subscript (mid[-2]) and a backward step from end() (*(end() - 1)). Because step_ is the signed difference_type, n * step_ stays signed and negative offsets step backwards rather than wrapping. Co-authored-by: Claude <noreply@anthropic.com>
IvanaGyro
force-pushed
the
claude/stride-view
branch
from
d85fd93 to
1e86067
Compare
|
Pushed
Left open for your call: the Generated by Claude Code |
2 participants
Summary
Adds
cytnx::linalg_internal::stride_view<V>, a strided random-access range adaptor that wraps any sized random-access rangeVand exposes everystep-th element as a fresh random-access range. Ther | stride(k)pipe form lets range algorithms consume strided slices directly.The iterator stores the underlying base iterator (not a pointer back to the view), so iterators stay valid as long as the underlying data lives — independent of the view's lifetime.
Why now
The next Trace refactor needs to reduce a tensor diagonal in place without first cloning the data into a contiguous buffer. Landing
stride_viewas its own change makes the iterator semantics reviewable in isolation and lets the diagonal-sum step in the trace PR focus on the trace algorithm itself.Tests
tests/linalg_test/stride_view_test.cppcovers:static_assert(std::random_access_iterator<...>)/std::ranges::random_access_range<...>/std::ranges::sized_range<...>.n % step != 0branch ofsize()).step == 0precondition (EXPECT_THROW(stride_view(..., 0), std::invalid_argument)).r | stride(k)) fed intoPairwiseSumto exercise the integration point.+ n,[n],end - begin,<).The test reaches into the internal header through a new
${CMAKE_SOURCE_DIR}/srcPRIVATE include on the test binary, so internal utilities can be unit-tested directly.Test plan
openblas-cpu: full suite 955/955 passed.mkl-cpu: full suite 955/955 passed.pre-commit(clang-format-14 + EOF + whitespace) clean.Draft — opened for review.
Generated by Claude Code