xtensor icon indicating copy to clipboard operation
xtensor copied to clipboard

Compiler error while accessing a 2D view of a 3D array by fixating the 3rd dimension (static_assert(I < sizeof...(Args), "I should be lesser than sizeof...(Args)");).

Open goto40 opened this issue 4 years ago • 12 comments

I found a strange asymmetry while playing with 3d xarrays and views on such arrays. Maybe I misunderstood the way xt::view works. Any hint on what happened is very appreciated.

Effect: While accessing 2D views of the 3D array I found that selecting a specific third dimension does not compile, while the same works for the first and second dimension (see example below).

For the third dimension I get a static_assert: I < sizeof...(Args), "I should be lesser than sizeof...(Args)".

My example can be explored by pasting the code below into the binder link from the xtensor docs (https://hub.mybinder.turing.ac.uk/user/xtensor-stack-xtensor-j9zqiixr/notebooks/notebooks/xtensor.ipynb):

#include <xtensor/xarray.hpp>
#include <xtensor/xview.hpp>

xt::xarray<float> a3d = xt::zeros<float>({10, 10, 10});
auto a0 = xt::view(a3d, 0, xt::all(), xt::all());
auto a1 = xt::view(a3d, xt::all(), 1, xt::all());
auto a2 = xt::view(a3d, xt::all(), xt::all(), 2);
a0(0, 0) = 1;    // ok
a1(0, 0) = 0.75; // ok
a2(0, 0) = 0.5;  // produces "static_assert(I < sizeof...(Args), "I should be lesser than sizeof...(Args)");"

In my setup I also check the correct change of the values in the underlying xarray... (I use f3c11b2d810159e7063daddeaa0764f4006e5a73 from the master branch of xtensor)

#include <xtensor/xarray.hpp>
#include <xtensor/xview.hpp>
//...
TEST_CASE("first steps: a3d array", "[xtensor]")
{
  xt::xarray<float> a3d = xt::zeros<float>({10, 10, 10});
  auto a0 = xt::view(a3d, 0, xt::all(), xt::all());
  auto a1 = xt::view(a3d, xt::all(), 1, xt::all());
  auto a2 = xt::view(a3d, xt::all(), xt::all(), 2);
  a0(0, 0) = 1;    // ok
  a1(0, 0) = 0.75; // ok
  a2(0, 0) = 0.5;  // produces "static_assert(I < sizeof...(Args), "I should be lesser than sizeof...(Args)");"

  CHECK(a3d(0, 0, 0) == Approx(1.0));
  CHECK(a3d(0, 1, 0) == Approx(0.75));
  CHECK(a3d(0, 0, 2) == Approx(0.5));
}

goto40 avatar Jun 06 '21 12:06 goto40

Thanks for the report. That is an unlikely error indeed, but I can confirm. For reference I list the entire error stream here:

.../include/xtensor/xutils.hpp:264:9: error: static_assert failed due to requirement '1UL < sizeof...(Args)' "I should be lesser than sizeof...(Args)"
        static_assert(I < sizeof...(Args), "I should be lesser than sizeof...(Args)");
        ^             ~~~~~~~~~~~~~~~~~~~
.../include/xtensor/xview.hpp:1653:60: note: in instantiation of function template specialization 'xt::argument<1, unsigned long>' requested here
        return static_cast<size_type>(slice.derived_cast()(argument<I>(static_cast<ST>(arg), static_cast<ST>(args)...)));
                                                           ^
.../include/xtensor/xview.hpp:1630:16: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int>::sliced_access<1, xt::xall<unsigned long>, unsigned long>' requested here
        return sliced_access<I - integral_count_before<S...>(I) + newaxis_count_before<S...>(I + 1)>
               ^
.../include/xtensor/xview.hpp:1616:20: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int>::index<1, unsigned long>' requested here
        return m_e(index<I>(args...)...);
                   ^
.../include/xtensor/xview.hpp:1578:16: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int>::access_impl<0, 1, unsigned long>' requested here
        return access_impl(make_index_sequence(arg, args...), arg, args...);
               ^
.../include/xtensor/xview.hpp:1576:20: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int>::access<unsigned long>' requested here
            return access(args...);
                   ^
.../include/xtensor/xview.hpp:1033:16: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int>::access<unsigned long, unsigned long>' requested here
        return access(static_cast<size_type>(args)...);
               ^
.../main.cpp:12:7: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int>::operator()<int, int>' requested here
    a2(0, 0) = 0.5;  // produces "static_assert(I < sizeof...(Args), "I should be lesser than sizeof...(Args)");"
      ^

tdegeus avatar Jun 07 '21 06:06 tdegeus

At the moment I do not feel that I have enough information to make a PR. With some help, I am willing to try my best:

in xview.hpp https://github.com/xtensor-stack/xtensor/blob/f3c11b2d810159e7063daddeaa0764f4006e5a73/include/xtensor/xview.hpp#L1572 I found a construct that I did not understand (because I have no architecture knowledge of the xtensor design):

    template <class CT, class... S>
    template <class Arg, class... Args>
    inline auto xview<CT, S...>::access(Arg arg, Args... args) -> reference
    {
        if (sizeof...(Args) >= this->dimension())        // <----- discard arg in some cases
        {
            return access(args...);
        }
        return access_impl(make_index_sequence(arg, args...), arg, args...);
    }

I did not understand why in some cases the arg is discarded (w/o error). I (experimentally) changed this check to throw an exception if too many arguments are passed ... My test works in that case, and I also uncovered another bug in my tests, where I passed to many indicies... But I am not sure if this "argument discarding" is by purpose for some internal cases... (open for discussion)

    template <class CT, class... S>
    template <class Arg, class... Args>
    inline auto xview<CT, S...>::access(Arg arg, Args... args) -> reference
    {
        if ((1 + sizeof...(Args)) > this->dimension())
        {
            throw std::runtime_error("unexpected TODO message");
        }
        /* I do not understand why we drop the frist argument here (w/o error)
        if (sizeof...(Args) >= this->dimension())
        {
            return access(args...);
        }*/
        return access_impl(make_index_sequence(arg, args...), arg, args...);
    }

Note: with the original code I was able to pass more indices than I have dimensions (in a view). All "extra indices" seem to be discarded as explained above....

goto40 avatar Jun 07 '21 17:06 goto40

Maybe this check can be done at compile time? Is there a compile time information about the number of dimensions?

goto40 avatar Jun 07 '21 18:06 goto40

At different locations in the tests, it seems crucial to allow to provide more that the required indices (e.g. 4 indices for a 2D array). In that case the supplementary indices are ignored. In the following cases I do not fully understand the use case (why to ignore supplementary indices): https://github.com/xtensor-stack/xtensor/blob/f3c11b2d810159e7063daddeaa0764f4006e5a73/test/test_xview.cpp#L337 https://github.com/xtensor-stack/xtensor/blob/f3c11b2d810159e7063daddeaa0764f4006e5a73/test/test_xview.cpp#L342 https://github.com/xtensor-stack/xtensor/blob/f3c11b2d810159e7063daddeaa0764f4006e5a73/test/test_xview.cpp#L347

In some cases with 0-dimensional views, accessing with one index with value 0 (special case) looks plausible: https://github.com/xtensor-stack/xtensor/blob/f3c11b2d810159e7063daddeaa0764f4006e5a73/test/test_xindex_view.cpp#L126

Note: I created a branch on my fork to make my experiments: https://github.com/goto40/xtensor/tree/analysis/issue_2395_index_problem (there I commented out the problematic test lines in test_xview.cpp and changed the code to adapt the handling of indices; ctest passes).

goto40 avatar Jun 08 '21 19:06 goto40

Hi! So dumping an excess of indices is there for broadcasting I think, so that is indeed intended (public) use.

At the same time I'm not entirely sure what causes the error. I have been briefly looking and I didn't get any closer to the source. The problem is also that these static errors can sometimes be harder to debug, as one cannot print intermediate values.

Anyway, the problem is broader that 'just' 3d. E.g.

#include <xtensor/xarray.hpp>
#include <xtensor/xview.hpp>

int main()
{
    xt::xarray<float> a3d = xt::zeros<float>({10, 10, 10, 10});
    auto a2 = xt::view(a3d, xt::all(), xt::all(), xt::all(), 2);
    a2(0, 0, 0) = 0.5;
    return 0;
}

gives the same error:

.../include/xtensor/xutils.hpp:264:9: error: static_assert failed due to requirement '1UL < sizeof...(Args)' "I should be lesser than sizeof...(Args)"
        static_assert(I < sizeof...(Args), "I should be lesser than sizeof...(Args)");
        ^             ~~~~~~~~~~~~~~~~~~~
.../include/xtensor/xview.hpp:1653:60: note: in instantiation of function template specialization 'xt::argument<1, unsigned long>' requested here
        return static_cast<size_type>(slice.derived_cast()(argument<I>(static_cast<ST>(arg), static_cast<ST>(args)...)));
                                                           ^
.../include/xtensor/xview.hpp:1630:16: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::sliced_access<1, xt::xall<unsigned long>, unsigned long>' requested here
        return sliced_access<I - integral_count_before<S...>(I) + newaxis_count_before<S...>(I + 1)>
               ^
.../include/xtensor/xview.hpp:1616:20: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::index<1, unsigned long>' requested here
        return m_e(index<I>(args...)...);
                   ^
.../include/xtensor/xview.hpp:1578:16: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::access_impl<0, 1, unsigned long>' requested here
        return access_impl(make_index_sequence(arg, args...), arg, args...);
               ^
.../include/xtensor/xview.hpp:1576:20: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::access<unsigned long>' requested here
            return access(args...);
                   ^
.../include/xtensor/xview.hpp:1576:20: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::access<unsigned long, unsigned long>' requested here
.../include/xtensor/xview.hpp:1033:16: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::access<unsigned long, unsigned long, unsigned long>' requested here
        return access(static_cast<size_type>(args)...);
               ^
.../main.cpp:8:7: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::operator()<int, int, int>' requested here
    a2(0, 0, 0) = 0.5;
      ^

and it is not even related to the last axis:

#include <xtensor/xarray.hpp>
#include <xtensor/xview.hpp>

int main()
{
    xt::xarray<float> a3d = xt::zeros<float>({10, 10, 10, 10});
    auto a2 = xt::view(a3d, xt::all(), xt::all(), 2, xt::all());
    a2(0, 0, 0) = 0.5;
    return 0;
}

also gives the same error:

.../include/xtensor/xutils.hpp:264:9: error: static_assert failed due to requirement '1UL < sizeof...(Args)' "I should be lesser than sizeof...(Args)"
        static_assert(I < sizeof...(Args), "I should be lesser than sizeof...(Args)");
        ^             ~~~~~~~~~~~~~~~~~~~
.../include/xtensor/xview.hpp:1653:60: note: in instantiation of function template specialization 'xt::argument<1, unsigned long>' requested here
        return static_cast<size_type>(slice.derived_cast()(argument<I>(static_cast<ST>(arg), static_cast<ST>(args)...)));
                                                           ^
.../include/xtensor/xview.hpp:1630:16: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::sliced_access<1, xt::xall<unsigned long>, unsigned long>' requested here
        return sliced_access<I - integral_count_before<S...>(I) + newaxis_count_before<S...>(I + 1)>
               ^
.../include/xtensor/xview.hpp:1616:20: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::index<1, unsigned long>' requested here
        return m_e(index<I>(args...)...);
                   ^
.../include/xtensor/xview.hpp:1578:16: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::access_impl<0, 1, unsigned long>' requested here
        return access_impl(make_index_sequence(arg, args...), arg, args...);
               ^
.../include/xtensor/xview.hpp:1576:20: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::access<unsigned long>' requested here
            return access(args...);
                   ^
.../include/xtensor/xview.hpp:1576:20: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::access<unsigned long, unsigned long>' requested here
.../include/xtensor/xview.hpp:1033:16: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::access<unsigned long, unsigned long, unsigned long>' requested here
        return access(static_cast<size_type>(args)...);
               ^
.../main.cpp:8:7: note: in instantiation of function template specialization 'xt::xview<xt::xarray_container<xt::uvector<float>, xt::layout_type::row_major, xt::svector<unsigned long, 4, std::__1::allocator<unsigned long>, true>> &, xt::xall<unsigned long>, xt::xall<unsigned long>, int, xt::xall<unsigned long>>::operator()<int, int, int>' requested here
    a2(0, 0, 0) = 0.5;
      ^

Maybe someone with more knowledge of this part of the code can comment @JohanMabille @SylvainCorlay

tdegeus avatar Jun 09 '21 07:06 tdegeus

I prepared a simple regression test (which can be put into test_xview.cpp:

    TEST(xview, regression_2395_access_compiler_problem)
    {
        // just checking it compiles...
        xt::xarray<float> a4d = xt::zeros<float>({10, 10, 10, 10});
        auto a4 = xt::view(a4d, xt::all(), xt::all(), 2, xt::all());
        a4(0, 0, 0) = 0.5;

        // similar test with some content checks...
        xt::xarray<int> a3d = xt::zeros<int>({10, 10, 10});
        auto a0 = xt::view(a3d, 0, xt::all(), xt::all());
        auto a1 = xt::view(a3d, xt::all(), 1, xt::all());
        auto a2 = xt::view(a3d, xt::all(), xt::all(), 2);
        a0(0, 0) = 1; // ok
        a1(0, 0) = 2; // ok
        a2(0, 0) = 3; // produced "static_assert(I < sizeof...(Args), "I should be lesser than sizeof...(Args)");"

        EXPECT_EQ(a3d(0, 0, 0), 1);
        EXPECT_EQ(a3d(0, 1, 0), 2);
        EXPECT_EQ(a3d(0, 0, 2), 3);
    }

goto40 avatar Jun 09 '21 17:06 goto40

Thanks @goto40 . A start of a PR for a fix could be to include this test. Once we work on the fix this would give us the all clear.

tdegeus avatar Jun 09 '21 17:06 tdegeus

I found a possible hint: Addding some C++17 compile-time if to prevent instantiating the template-access(args...)-method, I was able to compile the test above (diff: https://github.com/goto40/xtensor/commit/d326fde8586eae37ac5518dfb88649879157d3ae):

    template <class CT, class... S>
    template <class Arg, class... Args>
    inline auto xview<CT, S...>::access(Arg arg, Args... args) -> reference
    {
        constexpr size_t dim = sizeof...(S) - integral_count<S...>();       // NEW not 100% this->dimension()
        if constexpr (sizeof...(Args) >= dim)                               // NEW C++17 constexpr-if
        {
            if (sizeof...(Args) >= this->dimension())
            {
                return access(args...);
            }
        }
        return access_impl(make_index_sequence(arg, args...), arg, args...);
    }

In any case, the instantiation of the access method, never used and protected by the runtime-if, is a problem.

  • Note, that there is also a const version of that function.
  • The C++17 constexpr can be back-ported to C++14 with other constructs... (either some specialization or a SFINAE trick).
  • Ugly: the compiler-time dim in the code above does not always match the this->dimenson() value. Maybe another compile-time decision can prevent the instantiation of the erroneous construct.
  • I feel I did not 100% identify the problem....

I would appreciate some more help from the core team. The current state does not look mature enough for a PR. I work on my fork... If you wish I could make a PR with the unittest as suggested above - but I feel better to wait until we have gained deeper insights...

goto40 avatar Jun 09 '21 20:06 goto40

#2398 works perfectly for me. I just tried my example application and the bug is fixed - the software works... :-) @tdegeus, if you find time, maybe you could have a look if the fix makes sense to you. I am happy to discuss...

goto40 avatar Jun 18 '21 15:06 goto40

I figured I'll bump this up, since the issue is still present. And I think I understand why it happens, but I'm not so sure what the right solution is.

As mentioned here and in the associated #2398, one gets a compile error for

  xt::xtensor<int, 3> a3d = xt::zeros<int>({3, 3, 3});
  auto a0 = xt::view(a3d, 0, xt::all(), xt::all());
  auto a1 = xt::view(a3d, xt::all(), 1, xt::all());
  auto a2 = xt::view(a3d, xt::all(), xt::all(), 2);
  a0(0, 0) = 1; // ok 
  a1(1, 1) = 2; // ok
  a2(2, 2) = 3; // produced "xview.hpp:1700:25: error: static assertion failed: I should be less than 1 + sizeof...(Args)"

One way to fix this in this particular case is to do

diff --git a/include/xtensor/xview.hpp b/include/xtensor/xview.hpp
index 0a2dbb6b..f73a3329 100644
--- a/include/xtensor/xview.hpp
+++ b/include/xtensor/xview.hpp
@@ -1596,7 +1596,7 @@ namespace xt
     template <class Arg, class... Args>
     inline auto xview<CT, S...>::access(Arg arg, Args... args) -> reference
     {
-        if (sizeof...(Args) >= this->dimension())
+        if constexpr (sizeof...(Args) >= get_rank<self_type>::value)
         {
             return access(args...);
         }

But, besides using C++17 if constexpr which could be fixed, this only works for a view of an xtensor, not an xarray.

What happens in the case of the runtime if is that the code doesn't yet know whether we might be accessing the rank-2 view maybe with, say, 3 indices. In that case, it'll drop indices from the front until it gets a valid number of indices using this if statement.

However, when accessing the view with the correct number of arguments (2 in this example), the compiler still generates code for the path that 2 arguments may be too many, that is, it generates the code path where the first argument has been dropped. (I think in particular in the xtensor case, where it I suppose the optimizer can determine that this->dimension() == 2 at compile time, it'll then drop the other path, but it's too late, because the other path has generated the static assertion).

A simpler way to produce the problem directly is to do a2(2) = 3; -- that is, access the slice with too few args. I think the first question is, what should happen in this case? Given that a regular array access would pre-pad with zeros, it'd be consistent to use the same behavior, but that is currently not the case, for views more generally:

  • a2(2) = 3 gives a compile time error, essentially what started this issue.
  • If one does auto a0 = xt::view(a3d, 1, xt::all(), xt::all());, and then a0(2) = 2, one doesn't get a compile time error -- yet it also doesn't work right, ie., one doesn't get the same result as a0(0, 2) = 2. (What happens is that the compiler translates (2) using the first two slices into the index (1, 2), so one gets a3d(1, 2), and a3d turns that into a3d(0, 1, 2) by pre-padding, but that's not what one would have expected -- it should have become a3d(1, 0, 2).

In any case, I think I understand the code pretty well where this goes wrong, but I'm not so sure how involved it is to fix this, assuming that the desired behavior would be to provide consistent behavior for too many / too few indices to regular array access. An easier fix would be to not support accessing a slice with too many / too few indices in the first place.

germasch avatar Apr 01 '23 03:04 germasch

So as I had a chance to look at this a little more, first of all the suggest partial fix above

-        if (sizeof...(Args) >= this->dimension())
+        if constexpr (sizeof...(Args) >= get_rank<self_type>::value)

while it worked in this particular case is wrong -- I had assumed that if the rank of the underlying object is known at compile time, the rank of the view would be known at compile time through get_rank as well, and that is not the case (though it would be possible to determine it at compile time).

More generally, I've come to the conclusion that the current way that xview::access() is implemented makes it impossible to support the "too few arguments" case correctly, as it basically chooses to forward correspondingly too few arguments to the underlying object, and let's the underlying object pre-pad zeros. That, however, may put the zeros in the wrong place. Let me try to explain this again:

  xt::xtensor<int, 3> a3d = xt::zeros<int>({3, 3, 3});
  auto a0 = xt::view(a3d, 1, xt::all(), xt::all());
  EXPECT_EQ(&a0(0, 2), &a3d(1, 0, 2));
  EXPECT_EQ(&a0(2), &a3d(1, 0, 2));

The first EXPECT, which provides all arguments works just fine. What happens is that 3 indices are provided to the underlying argument using the 3 slices used when making the view:

I  slice  argument                        => index passed to underlying
0  1      #0 ( == 0), not actually used   1
1  all    #0 ( == 0)                      0
2  all    #1 ( == 2)                      2

So a0(0, 2) gets translated into a3d(1, 0, 2) as expected.

In the second case a0(2), since only one argument is passed, only two indices are passed to the underlying object (the "squeeze" 1, and the one argument given processed by the first xt::all()).

I  slice  argument                        => index passed to underlying
0  1      #0 ( == 2), not actually used   1
1  all    #0 ( == 2)                      2

So a0(2) becomes a3d(1, 2), and by pre-padding, that becomes a3d(0, 1, 2) and I don't think that is what one would expect. Rather, one would expect &a0(2) == &a0(0, 2) == &a3d(1, 0, 2).

The only way I see to fix this is to take care of the pre-padding on the xview side -- by the time too few indices have passed on to the underlying object, it's too late to get it right.

It's actually fairly straightforward, if a bit repetitive, to handle this by this kind of change:

@@ -1596,6 +1601,12 @@ namespace xt
     template <class Arg, class... Args>
     inline auto xview<CT, S...>::access(Arg arg, Args... args) -> reference
     {
+        constexpr std::size_t nargs_min = sizeof...(S) -
+          integral_count_before<S...>(sizeof...(S));
+        constexpr std::size_t nargs = 1 + sizeof...(Args);
+        if constexpr (nargs < nargs_min) {
+            return access(0, arg, args...);
+        }
         if (sizeof...(Args) >= this->dimension())
         {
             return access(args...);

If this looks like an acceptable way of fixing this issue, I can turn this into a PR (and make it work with C++14).

Even though it's 5 years ago, @JohanMabille looks to be the last person to have dealt with this problem space, if on the opposite case (too many arguments rather than too few).

germasch avatar Apr 08 '23 03:04 germasch

@germasch Thanks for the investigation and the detailed explanation! Your fix would be super welcome!

JohanMabille avatar Apr 12 '23 07:04 JohanMabille