From e78a19f0333931a4f24e5d7ece0c7e5447ffae82 Mon Sep 17 00:00:00 2001 From: Loic Gouarin Date: Thu, 29 Oct 2026 18:49:40 +0100 Subject: [PATCH] Fix xview_stepper on broadcast and reverse traversals xview_stepper is the stepper of views that are not strided, such as views with keep or drop slices. It had three bugs. step() and reset() received a dimension of the iterated shape but used it as a view dimension. When the view is broadcast to a higher rank, the first m_offset dimensions of that shape are not view dimensions, so the stepper moved along the wrong axes. reset() did not even skip them. With a = {{0, 1, 2, 3}, {4, 5, 6, 7}, {8, 9, 10, 11}}, zeros({2, 2, 2}) + view(a, keep(0, 2), keep(1, 3)) gave {{1, 1}, {3, 3}} twice instead of {{1, 3}, {9, 11}}, and other shapes read the strides of the underlying container out of bounds. m_index_keeper is indexed like the slices in the constructor and in to_end(), but step() and reset() indexed it by view dimension. The two differ when an integral slice comes first, so view(a, 1, keep(0, 2, 3)) iterated backward from an uninitialized entry and crashed. to_end() stepped back from the end of the underlying expression, which leaves the stepper one past the last element along the last underlying axis. That is not the innermost axis of the view when the last slice is integral, when it is a newaxis or when the view has fewer slices than dimensions, so rbegin() started at the wrong element or past the end. The stepper now subtracts m_offset like xstepper, indexes m_index_keeper by slice, delegates the dimensions without a slice to the underlying stepper, and builds the end position from the begin position, the way increment_stepper reaches it. to_begin() also moves to the first element of the view, not of the underlying expression. The reverse column major check in test_view_iter, disabled because it failed for keep slices, now passes and is enabled again. The new xview tests fail on master and pass with the fix, also under AddressSanitizer and UBSan. This also removes the GCC -Warray-bounds warnings reported in common_step_forward and common_reset. --- include/xtensor/views/xview.hpp | 304 +++++++++++--------------------- test/test_xview.cpp | 55 +++++- 2 files changed, 153 insertions(+), 206 deletions(-) diff --git a/include/xtensor/views/xview.hpp b/include/xtensor/views/xview.hpp index c647572de..6bc9a44d0 100644 --- a/include/xtensor/views/xview.hpp +++ b/include/xtensor/views/xview.hpp @@ -817,25 +817,14 @@ namespace xt private: bool is_newaxis_slice(size_type index) const noexcept; - void to_end_impl(layout_type l); - - template - void common_step_forward(size_type dim, F f); - template - void common_step_backward(size_type dim, F f); - - template - void common_step_forward(size_type dim, size_type n, F f); - template - void common_step_backward(size_type dim, size_type n, F f); - - template - void common_reset(size_type dim, F f, bool backwards); + void to_slice_begin(); + void common_reset(size_type dim, bool backwards); view_type* p_view; substepper_type m_it; size_type m_offset; - std::array m_index_keeper; + // Position of the stepper in each slice, indexed like the slices. + std::array m_index_keeper{}; }; // meta-function returning the shape type for an xview @@ -2010,26 +1999,13 @@ namespace xt , m_it(it) , m_offset(offset) { - if (!end) + if (end) { - std::fill(m_index_keeper.begin(), m_index_keeper.end(), 0); - auto func = [](const auto& s) noexcept - { - return xt::value(s, 0); - }; - for (size_type i = 0; i < sizeof...(S); ++i) - { - if (!is_newaxis_slice(i)) - { - size_type s = apply(i, func, p_view->slices()); - size_type index = i - newaxis_count_before(i); - m_it.step(index, s); - } - } + to_end(l); } else { - to_end_impl(l); + to_slice_begin(); } } @@ -2042,75 +2018,112 @@ namespace xt template inline void xview_stepper::step(size_type dim) { - auto func = [this](size_type index, size_type offset) - { - m_it.step(index, offset); - }; - common_step_forward(dim, func); + step(dim, size_type(1)); } template inline void xview_stepper::step_back(size_type dim) { - auto func = [this](size_type index, size_type offset) - { - m_it.step_back(index, offset); - }; - common_step_backward(dim, func); + step_back(dim, size_type(1)); } + // dim is a dimension of the (possibly broadcast) shape being iterated; the + // m_offset leading dimensions of that shape are not dimensions of the view. template inline void xview_stepper::step(size_type dim, size_type n) { - auto func = [this](size_type index, size_type offset) + if (dim < m_offset) { - m_it.step(index, offset); - }; - common_step_forward(dim, n, func); + return; + } + const size_type index = integral_skip(dim - m_offset); + if (index >= sizeof...(S)) + { + m_it.step(index - newaxis_count(), n); + } + else if (!is_newaxis_slice(index)) + { + auto func = [this, index, n](const auto& s) noexcept + { + const size_type st = step_size(s, m_index_keeper[index], n); + m_index_keeper[index] += n; + return st; + }; + m_it.step(index - newaxis_count_before(index), apply(index, func, p_view->slices())); + } } template inline void xview_stepper::step_back(size_type dim, size_type n) { - auto func = [this](size_type index, size_type offset) + if (dim < m_offset) { - m_it.step_back(index, offset); - }; - common_step_backward(dim, n, func); + return; + } + const size_type index = integral_skip(dim - m_offset); + if (index >= sizeof...(S)) + { + m_it.step_back(index - newaxis_count(), n); + } + else if (!is_newaxis_slice(index)) + { + auto func = [this, index, n](const auto& s) noexcept + { + m_index_keeper[index] -= n; + return step_size(s, m_index_keeper[index], n); + }; + m_it.step_back( + index - newaxis_count_before(index), + apply(index, func, p_view->slices()) + ); + } } template inline void xview_stepper::reset(size_type dim) { - auto func = [this](size_type index, size_type offset) - { - m_it.step_back(index, offset); - }; - common_reset(dim, func, false); + common_reset(dim, false); } template inline void xview_stepper::reset_back(size_type dim) { - auto func = [this](size_type index, size_type offset) - { - m_it.step(index, offset); - }; - common_reset(dim, func, true); + common_reset(dim, true); } template inline void xview_stepper::to_begin() { - std::fill(m_index_keeper.begin(), m_index_keeper.end(), 0); m_it.to_begin(); + to_slice_begin(); } + // Leaves the stepper where increment_stepper would leave it after the last + // element: one step past it along the innermost dimension of the traversal. template inline void xview_stepper::to_end(layout_type l) { - m_it.to_end(l); - to_end_impl(l); + if (l != layout_type::row_major && l != layout_type::column_major) + { + XTENSOR_THROW(std::runtime_error, "Iteration only allowed in row or column major."); + } + to_begin(); + if (p_view->size() == 0) + { + return; + } + const auto& shape = p_view->shape(); + const size_type dimension = shape.size(); + for (size_type d = 0; d < dimension; ++d) + { + step(m_offset + d, shape[d] - 1); + } + if (dimension != 0) + { + // In column major order the innermost dimension is the first one of the + // iterated shape, which is not a view dimension when m_offset != 0. + step(l == layout_type::row_major ? m_offset + dimension - 1 : size_type(0)); + } } template @@ -2120,175 +2133,66 @@ namespace xt return newaxis_count_before(index + 1) != newaxis_count_before(index); } + // Moves m_it from the first element of the underlying expression to the + // first element of the view. template - inline void xview_stepper::to_end_impl(layout_type l) + inline void xview_stepper::to_slice_begin() { + m_index_keeper.fill(0); auto func = [](const auto& s) noexcept { - return xt::value(s, get_size(s) - 1); - }; - auto size_func = [](const auto& s) noexcept - { - return get_size(s); + return xt::value(s, 0); }; - for (size_type i = 0; i < sizeof...(S); ++i) { if (!is_newaxis_slice(i)) { size_type s = apply(i, func, p_view->slices()); - size_type ix = apply(i, size_func, p_view->slices()); - m_index_keeper[i] = ix - size_type(1); - size_type index = i - newaxis_count_before(i); - s = p_view->underlying_size(index) - 1 - s; - m_it.step_back(index, s); + m_it.step(i - newaxis_count_before(i), s); } } - if (l == layout_type::row_major) - { - for (size_type i = sizeof...(S); i > 0; --i) - { - if (!is_newaxis_slice(i - 1)) - { - m_index_keeper[i - 1]++; - break; - } - } - } - else if (l == layout_type::column_major) - { - for (size_type i = 0; i < sizeof...(S); ++i) - { - if (!is_newaxis_slice(i)) - { - m_index_keeper[i]++; - break; - } - } - } - else - { - XTENSOR_THROW(std::runtime_error, "Iteration only allowed in row or column major."); - } } template - template - void xview_stepper::common_step_forward(size_type dim, F f) + inline void xview_stepper::common_reset(size_type dim, bool backwards) { - if (dim >= m_offset) + if (dim < m_offset) { - auto func = [&dim, this](const auto& s) noexcept - { - return step_size(s, this->m_index_keeper[dim]++, 1); - }; - size_type index = integral_skip(dim); - if (!is_newaxis_slice(index)) - { - size_type step_size = index < sizeof...(S) ? apply(index, func, p_view->slices()) - : 1; - index -= newaxis_count_before(index); - f(index, step_size); - } + return; } - } - - template - template - void xview_stepper::common_step_forward(size_type dim, size_type n, F f) - { - if (dim >= m_offset) + const size_type index = integral_skip(dim - m_offset); + if (index >= sizeof...(S)) { - auto func = [&dim, &n, this](const auto& s) noexcept + // Not covered by a slice: the view spans this whole dimension of the + // underlying expression, whose stepper knows its extent. + const size_type underlying_dim = index - newaxis_count(); + if (backwards) { - auto st_size = step_size(s, this->m_index_keeper[dim], n); - this->m_index_keeper[dim] += n; - return size_type(st_size); - }; - - size_type index = integral_skip(dim); - if (!is_newaxis_slice(index)) - { - size_type step_size = index < sizeof...(S) ? apply(index, func, p_view->slices()) - : n; - index -= newaxis_count_before(index); - f(index, step_size); + m_it.reset_back(underlying_dim); } - } - } - - template - template - void xview_stepper::common_step_backward(size_type dim, F f) - { - if (dim >= m_offset) - { - auto func = [&dim, this](const auto& s) noexcept - { - this->m_index_keeper[dim]--; - return step_size(s, this->m_index_keeper[dim], 1); - }; - size_type index = integral_skip(dim); - if (!is_newaxis_slice(index)) + else { - size_type step_size = index < sizeof...(S) ? apply(index, func, p_view->slices()) - : 1; - index -= newaxis_count_before(index); - f(index, step_size); + m_it.reset(underlying_dim); } } - } - - template - template - void xview_stepper::common_step_backward(size_type dim, size_type n, F f) - { - if (dim >= m_offset) + else if (!is_newaxis_slice(index)) { - auto func = [&dim, &n, this](const auto& s) noexcept + auto func = [this, index, backwards](const auto& s) noexcept { - this->m_index_keeper[dim] -= n; - return step_size(s, this->m_index_keeper[dim], n); + const size_type size = get_size(s); + m_index_keeper[index] = backwards ? size - 1 : 0; + return xt::value(s, size - 1) - xt::value(s, 0); }; - - size_type index = integral_skip(dim); - if (!is_newaxis_slice(index)) + const size_type underlying_dim = index - newaxis_count_before(index); + const size_type n = apply(index, func, p_view->slices()); + if (backwards) { - size_type step_size = index < sizeof...(S) ? apply(index, func, p_view->slices()) - : n; - index -= newaxis_count_before(index); - f(index, step_size); + m_it.step(underlying_dim, n); } - } - } - - template - template - void xview_stepper::common_reset(size_type dim, F f, bool backwards) - { - auto size_func = [](const auto& s) noexcept - { - return get_size(s); - }; - auto end_func = [](const auto& s) noexcept - { - return xt::value(s, get_size(s) - 1) - xt::value(s, 0); - }; - - size_type index = integral_skip(dim); - if (!is_newaxis_slice(index)) - { - if (dim < m_index_keeper.size()) + else { - size_type size = index < sizeof...(S) ? apply(index, size_func, p_view->slices()) - : p_view->shape()[dim]; - m_index_keeper[dim] = backwards ? size - 1 : 0; + m_it.step_back(underlying_dim, n); } - - size_type reset_n = index < sizeof...(S) ? apply(index, end_func, p_view->slices()) - : p_view->shape()[dim] - 1; - index -= newaxis_count_before(index); - f(index, reset_n); } } } diff --git a/test/test_xview.cpp b/test/test_xview.cpp index 8069bb016..69cca47aa 100644 --- a/test/test_xview.cpp +++ b/test/test_xview.cpp @@ -1117,16 +1117,14 @@ namespace xt ++riter_expv1; } - // TODO: this test cannot pass for views containing squeezes of - // keep slices with a single element. This is NOT trivial to fix, - // but this use case is not that common. - /*auto rciter_expv1 = exp.template rbegin(); + auto rciter_expv1 = exp.template rbegin(); for (auto iter = v.template rbegin(); - iter != v.template rend(); ++iter) + iter != v.template rend(); + ++iter) { EXPECT_EQ(*iter, *rciter_expv1); ++rciter_expv1; - }*/ + } } TEST(xview, random_stepper) @@ -1221,6 +1219,51 @@ namespace xt EXPECT_EQ(res, exp); } + TEST(xview, keep_slice_reverse_iteration) + { + xtensor a = {{0, 1, 2, 3}, {4, 5, 6, 7}, {8, 9, 10, 11}}; + + auto v1 = xt::view(a, 1, keep(0, 2, 3)); + xtensor exp_v1 = {4, 6, 7}; + test_view_iter(v1, exp_v1); + + auto v2 = xt::view(a, keep(0, 2), 1); + xtensor exp_v2 = {1, 9}; + test_view_iter(v2, exp_v2); + + auto v3 = xt::view(a, keep(0, 2)); + xtensor exp_v3 = {{0, 1, 2, 3}, {8, 9, 10, 11}}; + test_view_iter(v3, exp_v3); + + auto v4 = xt::view(a, keep(0, 2), xt::all(), xt::newaxis()); + xtensor exp_v4 = {{{0}, {1}, {2}, {3}}, {{8}, {9}, {10}, {11}}}; + test_view_iter(v4, exp_v4); + + auto v5 = xt::view(a, xt::newaxis(), drop(1), 2); + xtensor exp_v5 = {{2, 10}}; + test_view_iter(v5, exp_v5); + } + + TEST(xview, keep_broadcast_higher_rank) + { + xtensor a = {{0, 1, 2, 3}, {4, 5, 6, 7}, {8, 9, 10, 11}}; + + auto v = xt::view(a, keep(0, 2), keep(1, 3)); + xtensor exp = {{{1, 3}, {9, 11}}, {{1, 3}, {9, 11}}}; + xtensor z = xt::zeros({2, 2, 2}); + xtensor res = z + v; + EXPECT_EQ(res, exp); + xtensor zc = xt::zeros({2, 2, 2}); + xtensor res_cm = zc + v; + EXPECT_EQ(res_cm, exp); + + auto w = xt::view(a, 1, keep(0, 2, 3)); + xtensor b = {{100, 200, 300}, {400, 500, 600}}; + xtensor res2 = b + w; + xtensor exp2 = {{104, 206, 307}, {404, 506, 607}}; + EXPECT_EQ(res2, exp2); + } + TEST(xview, drop_slice) { xtensor a = {