Add support for column-major grid order

[Shir0kamii]

Hi, I started working on my proposition from #33.

It is far from finished, but I wanted to open the PR as soon as possible, in case you have time to take a look and had early remarks to make.

[becheran]

Looks promising. Regarding the question whether having a constructor for column major order, I guess it does make sense to explicitly select the order.

I guess it does make sense to let the user choose. Either row or column operations will be fast depending on the selected order and I guess it depends on the application which of both is more relevant.

[Shir0kamii]

Hi!

I'm nearing the end of this, but there is a couple things I'd like your input on.

First, the macro. I'm wondering if it'd be better to have a different macro for each grid order, or a single macro with an optional argument.

Second, the indexing. I still think having grid[col][row] for column-major grids deviates too much from the rest of the API, where the row always come before the column. But the only alternative seem to be to remove it in favor of grid[(row, col)].

What's your opinion?

PS: I just noticed the addition of serde support, I'll rebase and incorporate it soon.

[becheran]

Thanks for the update. Sorry for the late reply.

Personaly l prefer having two macros instead of one with an additional argument.

You are right. I guess your suggestions of using grid[(row, col)] instead makes sense.

The serde support might need some minor adjustment to also contain the order info. Guess it should not be too hard to resolve the conflict.

Thanks for the pull request and remove the draft state once I shall review it.

[Shir0kamii]

Hi!

I finaly had the time to make a big review of my own changes to make sure I didn't forget anything.

I'll try to list the public changes I introduced:

• Added the Order enum
• Added grid2 macro (I'm unhappy with the name, but didn't find anything short enough to be used with macros)
• Added Grid::*_with_order contructors
• Added Grid::order accessor method
• Added Grid::flip_rows and Grid::flip_cols (necessary for the new rotate implementations)
• Modified the signatures of Grid::transpose and Grid::rotate_* methods
• Removed implementations of Index<usize> and IndexMut<usize>
• Updated documentation related to memory layout to include notes about default order, performances and change of order.

Of course, I modified the body of a lot of methods to account for the column-major memory layout, and almost doubled the number of tests.

It is a big PR, please take your time to review it, I'll stay available for any changes you'd like me to make.

[becheran]

Just started the review. Sorry for slow response. Will continue when I have time again. Hopefully soon. Looks very promissing so far!

[becheran]

I realy like the changes. It improves the create quiete a lot! Thanks for adding all the new unit tests as well. Very well done.

Plz have a look to the comments. On the question whether having a default order or not for grid I would like to hear your opinion.

One thing that I noted when I ran the benchmarks on my PC is that besides some nice performance improvements I also see a regressed performance in the grid_set function which I cannot really explain right now:

grid_set                time:   [4.4354 ns 4.5241 ns 4.6186 ns]
                        change: [+102.90% +110.30% +117.76%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
  2 (2.00%) low mild
  2 (2.00%) high mild
  2 (2.00%) high severe

Do you see any explanation for that? Or maybe it was just some hickup on my system. Maybe a "performance vs efficient core" issue when doing the measurements.

[Shir0kamii]

Thank you for the review!

Regarding the constructors: In my opinion some people will not care about performance and use the crate just for the abstraction part of it. To them, implementation detail such as the memory layout will not matter and it feels wrong to make them choose one explicitly. That's why I kept the old constructors and made row-major the default. For people who care about performances, I think the part I added in the global documentation is enough to nudge them in the good direction.

I ran the benchmark and also saw a performance regression on grid_set, although it was "only" 50% (as opposed to the 110% in your comment). I can't explain why either, but I'd blame some compiler optimization not happening anymore. Do you think it is worth investigating?

[becheran]

You convinced me with the default constructor. Will keep as you proposed.

I am not sure why grid_set should have changed much performance wise. It might also be just a measurement issue.