[WIP] Initial xstrided_view and adding layout template parameter #240

wolfv · 2017-04-04T06:12:36Z

This adds layout as template parameter for xarray so far, and an initial implementation for xstrided_view.

On top of xstrided_view there are basic implementations of transpose_view and diag_view.

…edview

wolfv · 2017-04-04T19:34:10Z

The biggest drawback is that those methods won't work on non-strided containers (e.g. xgenerators).

We could add methods to make it work by computing strides if necessary. Through xstridedview we could also provide a generic reshape method.

But maybe that's something for the future?

SylvainCorlay · 2017-04-05T10:36:07Z

Thanks for tackling this. This is on the critical path in the short term for a lot of things.

SylvainCorlay · 2017-04-05T10:37:54Z

include/xtensor/xcontainer.hpp

+            {
+                std::cout << "RESHAPING WITH COL_MAJOR" << std::endl;
+                reshape(shape, layout::column_major);
+            }


I guess that we should default to row_major, instead of defaulting to column_major when the layout type is custom.

Makes sense

…s, make initialization from initializer list for column layout work

wolfv · 2017-04-06T00:43:05Z

Ok, I've reworked this once more to use the layout enum as template parameter.

At this point I'd be happy for a review.

It still needs more tests I guess. For example, checking that xexpressions with column_major and row_major containers work as expected. If r is row_major and c is column_major, this fails:

c = {{1,2,3}, {4,5,6}};
r = {{1,2,3}, {4,5,6}};
all(equal(c, r));

while equal(c, r) actually prints the correct result. That is probably, because all uses cbegin and cend (it works with xbegin/end.
So I guess we could work around this by checking both expressions, and use the fast iterators only if both are equal, and otherwise fall back to a slow method.

I just think it might be a lot of work ...

Cheers,

Wolf

JohanMabille · 2017-04-06T05:15:41Z

include/xtensor/xcontainer.hpp

@@ -220,7 +190,7 @@ namespace xt
        using inner_strides_type = typename base_type::inner_strides_type;
        using inner_backstrides_type = typename base_type::inner_backstrides_type;

-        void reshape(const shape_type& shape);
+        void reshape(const shape_type& shape, bool force = false);
        void reshape(const shape_type& shape, layout l);
        void reshape(const shape_type& shape, const strides_type& strides);


The last two overloads should be available for dynamic layout only. Else you may end up with a container whose strides are not consistent with the layout.

we could also add static_asserts in the function to only allow reshaping with a different layout if L == dynamic? That would keep the interface consistent.

I find it strange to provide methods that may fail to compile, depending the template parameters you use. Most of the times, people need the reshape overload that takes the new shape only, other overloads are provided for tuning / optimization. Enabling these last ones for dynamic layout only doesn't make the interface more inconsistent thant providing reshape methods for containers only, for instance.

Maybe the cleanest way would be to provide these methods in a specialization of the strided_container class, but that would be a bit of overkill imo.

I think one benefit of static_asserts would be that we could print a helpful error message, e.g. "reshaping with layout parameter only possible if layout == dynamic".
I have changed it now, though, to disable reshaping with layout if L != dynamic.

Hmm, I tried this, but it requires rewriting almost all constructors for xarray and xtensor.

I guess that it means enabling/disabling them depending on the layout template parameter ? Let's fallback to static_asserts for now if it's less time consuming, we will potentially refactor them later.

JohanMabille · 2017-04-06T05:18:25Z

include/xtensor/xcontainer.hpp

+            else
+            {
+                reshape(shape, layout::row_major);
+            }
        }


The layout is know at compile time, so it might be better to rely on that and select the appropriated reshape with some metaprogramming tools (tag dispatching or specializations of private implementation structures)

I was hoping that the compiler would be smart enough to just remove the false statement anyways, but sure, we can do some metaprogramming :)

Indeed the compiler will remove the false statement, but it will check the syntax of every statement. This remark was to be consistent with assuming that the reshape overloads are enabled for dynamic layout only.

I have added a member variable m_layout which and changed the logic slightly (will be in the next commit).

JohanMabille · 2017-04-06T05:20:54Z

include/xtensor/xlayout.hpp

+    };
+}
+
+#endif


What is the reason for moving this out of xstrides.hpp ?

I think it isn't possible to forward declare an enum, but I needed the declaration in forward_xtensor. We could also put the enum in forward_xtensor.hpp. What do you think?

Actually starting with C++11 you can forward declare an enum as long as you provide the storage type. But since you need to provide a default layout for xtensor and xarray aliases, you need to know the enum definition in xtensor_forward.hpp. So it might be relevant to define it here (I prefer not to include any other header in xtensor_forward.hpp).

JohanMabille · 2017-04-06T05:22:34Z

include/xtensor/xstrides.hpp

@@ -14,15 +14,11 @@
 #include <numeric>

 #include "xexception.hpp"
+#include "xlayout.hpp"
+#include "xtensor_forward.hpp"



xtensor_forward.hpp should not be included here since we do not manipulate tensor types in this file. If this is required by other files including xstrides.hpp, then add the include in these files rather than in this one.

Thanks for the catch

If you define the layout enum in xtensor_forward.hpp I think you will entually keep the include here.

JohanMabille · 2017-04-06T05:25:18Z

include/xtensor/xtensor_forward.hpp

@@ -43,10 +46,10 @@ namespace xt
     * @tparam A The allocator of the container holding the elements.
     * @tparam SA The allocator of the containers holding the shape and the strides.
     */
-    template <class T, class A = std::allocator<T>, class SA = std::allocator<typename std::vector<T, A>::size_type>>
-    using xarray = xarray_container<DEFAULT_DATA_CONTAINER(T, A), DEFAULT_SHAPE_CONTAINER(T, A, SA)>;
+    template <class T, layout L = layout::row_major, class A = std::allocator<T>, class SA = std::allocator<typename std::vector<T, A>::size_type>>


Documentation should be updated

JohanMabille · 2017-04-06T05:25:34Z

include/xtensor/xtensor_forward.hpp

@@ -68,8 +71,8 @@ namespace xt
     * @tparam N The dimension of the tensor.
     * @tparam A The allocator of the containers holding the elements.
     */
-    template <class T, std::size_t N, class A = std::allocator<T>>
-    using xtensor = xtensor_container<DEFAULT_DATA_CONTAINER(T, A), N>;
+    template <class T, std::size_t N, layout L = layout::row_major, class A = std::allocator<T>>


Here too :) The same for other types in this file

JohanMabille · 2017-04-06T05:28:18Z

include/xtensor/xarray.hpp

@@ -53,15 +53,15 @@ namespace xt
     * @tparam SC The type of the containers holding the shape and the strides.
     * @sa xarray
     */


Documentation should be updated, the same for types in xtensor.hpp

JohanMabille · 2017-04-06T05:43:01Z

include/xtensor/xstridedview.hpp

+        CT m_e;
+        const shape_type m_shape;
+        const strides_type m_strides;
+        const std::size_t m_offset;


Why making these members const ? This prevents you from reshaping and transposing in place. Not sure we want to allow that, though.

JohanMabille · 2017-04-06T05:47:08Z

include/xtensor/xcontainer.hpp

-        template <class I, std::size_t N, class Tag = check_policy::none>
-        void transpose(const I (&permutation)[N], Tag check_policy = Tag());
-#endif
-


I wonder if we should keep the transpose methods for containers with dynamic layout. That would allow "in-place" transposition, but may be confusing with the methods provided in xstridedview.hpp.

JohanMabille · 2017-04-06T06:08:54Z

@wolfv Thanks for implementing this, I made a quick review as you can see, I'll go into more details after pydata. Indeed there is still a lot of work to make the static layout working everywhere (assignment, iterators, functions), but I think it may be done in another PR; actually I have a good idea of how to do that and I need it to implement SIMD.

wolfv · 2017-04-06T06:27:50Z

@JohanMabille thanks! To remove all of the hacks in xblas it would be good if this lands as well :)
We should definitly coordinate how we want to go about this. If you think this has enough features to land in master with some minor fixes than I'd stop working on it and we fix the bunch of tests where col and row layout are mixed in a later PR?

JohanMabille · 2017-04-06T08:40:58Z

@wolfv if this PR is enough so that you can clean up xblas, we can merge it in master as soon as you have pushed the minor fixes we mentioned earlier. I will start to work on propagating the layout to assignment, iterators and functions just after our talk this sunday. We will update the documentation with more details about layout once everything is working.

Besides, that will fix #161 and #231.

Remove the additional `xml` folder generated by `doxygen`

Fixup docs clean target

…tests

wolfv · 2017-04-07T21:22:24Z

closing this in favor of the two new PR's.

wolfv added 4 commits April 3, 2017 23:10

add initial xstrided_view

c5e9db2

fix tests

e34d92d

add extended diagonal view

5154b00

finalize reimplementing diagonal, flip and transpose on top of xstrid…

67bfd16

…edview

add layout template parameter to xtensor

b1b90bd

SylvainCorlay reviewed Apr 5, 2017

View reviewed changes

wolfv added 2 commits April 5, 2017 16:00

rework layout template parameter to use enum directly, make tests pas…

88191fb

…s, make initialization from initializer list for column layout work

merge master

e4ddb19

JohanMabille reviewed Apr 6, 2017

View reviewed changes

ghisvail and others added 7 commits April 6, 2017 14:01

Fixup docs clean target

34197a1

Remove the additional `xml` folder generated by `doxygen`

Merge pull request xtensor-stack#249 from ghisvail/patch-1

773855a

Fixup docs clean target

minor changes to reshape, more explicit layout template parameter in …

90d3615

…tests

merge master

64ba4ee

specifiy xt::layout

1e5e860

try to please msvc

7739d6f

fix another layout

d7ab6bb

SylvainCorlay force-pushed the master branch from 773855a to cee174c Compare April 6, 2017 23:15

wolfv added 2 commits April 6, 2017 22:04

add some tests for different layouts/assignment

ac557e9

add more tests to check support for columnar layouts

f0acd4e

wolfv closed this Apr 7, 2017

wolfv deleted the xstrided_view branch October 30, 2017 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Initial xstrided_view and adding layout template parameter #240

[WIP] Initial xstrided_view and adding layout template parameter #240

wolfv commented Apr 4, 2017 •

edited

Loading

wolfv commented Apr 4, 2017

SylvainCorlay commented Apr 5, 2017

SylvainCorlay Apr 5, 2017

wolfv Apr 5, 2017

wolfv commented Apr 6, 2017

JohanMabille Apr 6, 2017

wolfv Apr 6, 2017 •

edited

Loading

JohanMabille Apr 6, 2017

wolfv Apr 6, 2017

wolfv Apr 6, 2017

JohanMabille Apr 6, 2017 •

edited

Loading

JohanMabille Apr 6, 2017

wolfv Apr 6, 2017

JohanMabille Apr 6, 2017 •

edited

Loading

wolfv Apr 6, 2017

JohanMabille Apr 6, 2017

wolfv Apr 6, 2017

JohanMabille Apr 6, 2017 •

edited

Loading

JohanMabille Apr 6, 2017

wolfv Apr 6, 2017

JohanMabille Apr 6, 2017

JohanMabille Apr 6, 2017

wolfv Apr 6, 2017

JohanMabille Apr 6, 2017

JohanMabille Apr 6, 2017

JohanMabille Apr 6, 2017

JohanMabille Apr 6, 2017

JohanMabille commented Apr 6, 2017

wolfv commented Apr 6, 2017

JohanMabille commented Apr 6, 2017 •

edited

Loading

wolfv commented Apr 7, 2017

+                  };
+              }
+              #endif

[WIP] Initial xstrided_view and adding layout template parameter #240

[WIP] Initial xstrided_view and adding layout template parameter #240

Conversation

wolfv commented Apr 4, 2017 • edited Loading

wolfv commented Apr 4, 2017

SylvainCorlay commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolfv commented Apr 6, 2017

Choose a reason for hiding this comment

wolfv Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanMabille Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanMabille Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanMabille Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanMabille commented Apr 6, 2017

wolfv commented Apr 6, 2017

JohanMabille commented Apr 6, 2017 • edited Loading

wolfv commented Apr 7, 2017

wolfv commented Apr 4, 2017 •

edited

Loading

wolfv Apr 6, 2017 •

edited

Loading

JohanMabille Apr 6, 2017 •

edited

Loading

JohanMabille Apr 6, 2017 •

edited

Loading

JohanMabille Apr 6, 2017 •

edited

Loading

JohanMabille commented Apr 6, 2017 •

edited

Loading