Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements |

[ Thread Index | Date Index | More lists.tuxfamily.org/eigen Archives ]

*To*: eigen@xxxxxxxxxxxxxxxxxxx*Subject*: Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements*From*: Benoit Steiner <benoit.steiner.goog@xxxxxxxxx>*Date*: Wed, 11 Jun 2014 10:53:41 -0700*Dkim-signature*: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=LCZn6pBFh8RbATyACYA1VLxO+5/Jl/esxFlRpnUcPyM=; b=D7dwLfwfj1LJL0HoESJBc0hSZd571z1Bj4sxSqAmw7zXlbmi593/XsJS0TYzTThhYQ baNknEgrUTHBYVvA6uIzhK4Y0uB32b8GAybJTAQYvsjNRMQzapQFZvE1HAdxu8by1dEX omJgcwzc/UgE+h2ffJ0fl7Rr/nK4SoBfNmk7eDUN6QO0jhhmurzGYBJt8kq2/dbjaaQT 0jza84AvfkAja54jIXL8JZchU2XxHASa0h4tE4nkTOS63GBIBi025iYWNQmfshXExcNH alljFkqRhDKohh3CqFjkP47GigPd/yKufgSKeNRluvtrGfSWF6o52f+auQB8hrid+r1k CRUA==

My proposal for this issue was, to reiterate it in a bit more detail:

- merge the pull request

- create unsupported/Eigen/CXX11/Emulated module that includes the

required emulation stuff

- create unsupported/Eigen/CXX11/CoreOrEmulated that either includes

CXX11/Core or CXX11/Emulated depending on compiler version

- move tensor module to unsupported/Eigen/Tensor (one dir up)

- change tensor module to include

unsupported/Eigen/CXX11/CoreOrEmulated

- leave TensorSymmetry underneath CXX11 and have it include CXX11/Core

but not CoreOrEmulated

Sounds good to me.

This has the consequence that:

- Tensor module can now be used without C++11 support

#include <unsupported/Eigen/Tensor>

- Tensor module will automatically load advanced C++11 version if the

compiler supports it, but will fall back to the C++03 version if it

doesn't. Differences between both versions will be:

* C++03: maximum number of indices, C++11: unlimited

as I said before, we should choose a sensible limit, I think

that Fortran's seven (7) is a good idea (current limit for

Benoit's code is five (5))

I'll need 8 in the near future, so let's go broke ;)

* otherwise, I don't see anything in the feature set that's

specific to C++11 that doesn't require the user to actively

use C++11 themselves (such as initializer list constructors

etc.)

- TensorSymmetry module still needs C++11

#include <unsupported/Eigen/CXX11/TensorSymmetry>

But this is definitely something that can be done after the pull

request has been merged.

So the only case somebody uses something that's C++11, it's going to be

more than MAX_INDICES incides in a tensor. If we don't make MAX_INDICES

too small, I don't think this is going to be a huge issue.In C++11 std::array has a constexpr constructor and in C++14 its

c. Eigen::array is a bad name decision for emulating std::array,

since array kind-of-clashes with Eigen::Array - it's going to be

hell for people using Eigen's traditional capital-A Array

template if they accidentally have a typo.

Is there actually a practical difference between Eigen::Array<Type,

size, 1> and std::array<Type, size>? I.e., why not allow indexing with

the already existing Eigen::Array and optionally implement a direct

mapping from std::array if that is available? You'd also don't need to

re-implement constructors for fixed sized arrays (up to size 4 at the

moment, but we could easily increase that to 6 or 8, maybe)

accessors are also constexpr. CXX11Workarounds.h contains some

workarounds that allows C++11 compilers to have constexpr access to

array elements. On the other hand, this does not apply to Eigen::Array,

and I'm not sure how difficult it would be to do this change and be

vary of regressions later.

If it is the case that both constructor and accessors are constexpr,

then the compiler can easily optimize away a lot of things w.r.t. to

the calculation of the linear index from a set of indices and the such.

For example, the internal::array_prod I added is reduced to the correct

number of multiplication assembly instructions when calculating the

total number of elements in a tensor constructor if this is constexpr.

So, from a C++03 version of the Tensor module, where constexpr is not

available, there is indeed no reason not to use Eigen::Array. From a

C++11 standpoint, using Eigen::Array is probably far from optimal.

I use std::array in order to encode ordered lists of dimensions. I am not convinced that using Eigen::Array instead is a good choice:

* Eigen Array is a 2d object. Users will always wonder if they need to order their coefficients in a row or a column.

* The Array class lacks nice to have cxx11 features such as initialization lists. This is syntactic sugar, but if I can use a recent compiler I'd rather write:

Tensor<float, 3> T({1,2,3})

than

Array<ptrdiff_t, 3, 1> dims;

coeffs << 1, 2, 3;

Tensor<float, 3> T(dims);

* I'm being optimistic here but a tensor is a generalization of the functionality provided by the Array class. In the distant future it might be possible to reimplement the Array class as a special case of Tensor, as long as the tensor API doesn't depend on the Array class itself.

**Follow-Ups**:**Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements***From:*Christoph Hertzberg

**References**:**[eigen] [RFC] Tensor module: class hierarchy***From:*Christian Seiler

**Re: [eigen] [RFC] Tensor module: class hierarchy***From:*Benoit Steiner

**[eigen] [Review] Pull request 66, Huge Tensor module improvements***From:*Christian Seiler

**Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements***From:*Christoph Hertzberg

**Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements***From:*Christian Seiler

**Messages sorted by:**[ date | thread ]- Prev by Date:
**Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements** - Next by Date:
**Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements** - Previous by thread:
**Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements** - Next by thread:
**Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements**

Mail converted by MHonArc 2.6.19+ | http://listengine.tuxfamily.org/ |