kokkos / mdspan

Reference implementation of mdspan targeting C++23
Other
417 stars 69 forks source link

Wrong behavior in seemingly simple test #31

Closed ohmree closed 4 years ago

ohmree commented 4 years ago

Hi, this is probably my mistake but while using the (awfully convenient) mdspan I encountered unexpected functionality in my program.

I managed to get a minimal reproduction of the bug:

void test() {
    using Grid =
        stdex::mdspan<int, stdex::dynamic_extent, stdex::dynamic_extent>;

    // We want a 5x5 grid...
    size_t           width = 5, height = 5;
    std::vector<int> vec(width * height, 0);
    // ...but in C++ array indices start from 0
    Grid grid(vec.data(), width - 1, height - 1);

    for (int i = 0; i <= width; ++i) {
        for (int j = 0; j <= height; ++j) { std::cout << grid(i, j) << ' '; }
        std::cout << '\n';
    }

    std::cout << '\n';
    grid(1, 0) = 1;

    for (int i = 0; i <= width; ++i) {
        for (int j = 0; j <= height; ++j) { std::cout << grid(i, j) << ' '; }
        std::cout << '\n';
    }
}

Running test() produces the following output:

0 0 0 0 0 0 
0 0 0 0 0 0 
0 0 0 0 0 0 
0 0 0 0 0 0 
0 0 0 0 0 0 
0 0 0 0 0 0 

0 0 0 0 1 0 
1 0 0 0 0 0 
0 0 0 0 0 0 
0 0 0 0 0 0 
0 0 0 0 0 0 
0 0 0 0 0 0 

First of all, there's an extra 1 I never set, and in my more complex program this has been happening with some squares in a 5x5 grid (but not all of them). Am I doing something wrong?

Second of all, is the default indexing here row-major or column-major? I'm confused.

I was under the impression that [1, 0] meant "go one along the X axis, and then zero along the Y axis" (I believe this is row-major indexing?) but the results show that it's the opposite - "one along the Y axis and then zero along the X axis". So which one is it in this case?

Sorry for the beginner questions, and thanks in advance.

dalg24 commented 4 years ago

Your grid is not sized properly and you have out-of-bound accesses

--- old
+++ new
@@ -6,18 +6,18 @@
    size_t           width = 5, height = 5;
    std::vector<int> vec(width * height, 0);
    // ...but in C++ array indices start from 0
-   Grid grid(vec.data(), width - 1, height - 1);
+   Grid grid(vec.data(), width, height);

-   for (int i = 0; i <= width; ++i) {
-       for (int j = 0; j <= height; ++j) { std::cout << grid(i, j) << ' '; }
+   for (int i = 0; i < width; ++i) {
+       for (int j = 0; j < height; ++j) { std::cout << grid(i, j) << ' '; }
        std::cout << '\n';
    }

    std::cout << '\n';
    grid(1, 0) = 1;

-   for (int i = 0; i <= width; ++i) {
-       for (int j = 0; j <= height; ++j) { std::cout << grid(i, j) << ' '; }
+   for (int i = 0; i < width; ++i) {
+       for (int j = 0; j < height; ++j) { std::cout << grid(i, j) << ' '; }
        std::cout << '\n';
    }
 }
ohmree commented 4 years ago

Whoops, that's embarrassing. The code runs just fine after applying your fixes.

So the constructor takes dimensions starting from 1 and not 0 like I thought?

mhoemmen commented 4 years ago

So the constructor takes dimensions starting from 1 and not 0 like I thought?

It's just like std::vector: If you want N elements, you give its constructor N. Whether the indexing is zero-based or one-based doesn't matter for the constructor.