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*: Christoph Hertzberg <chtz@xxxxxxxxxxxxxxxxxxxxxxxx>*Date*: Wed, 11 Jun 2014 17:03:16 +0200

On 11.06.2014 15:39, Christian Seiler wrote:

Hi,b. Currently, since you detect the C++ version to decide which header to include, this breaks people (read: me ;-)) using TensorSymmetry/ if they accidentally compile it with C++ <= 03 - instead of getting a nice error message, they get lots of substitution errors, because the C++11 version of the header is not included.We had a similar discussion about adding C++11 features before. I think we somehow agreed to not silently include C++11 dependent features which could principally be implemented in C++03 somehow, without making it obvious to the user that he depends on C++11. That was the also the basic purpose of the Eigen/CXX11/... include structure.I don't quite understand your sentence "to not silently include C++11 dependent features which could principally be implemented in C++03 somehow".

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

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)In C++11 std::array has a constexpr constructor and in C++14 its 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.

int testEigen() { Eigen::Array4i A(0,1,2,3); return A(2)-A(3); } resulted basically in _Z9testEigenv: movl $-1, %eax ret

Of course, you will not be able to write things like enum {value = testEigen()};

4. General observations: a. For all evaluators, you assume something like LinearAccessBit, which is fine as long as one doesn't implement some kind of generalized .block() for tensor and/or Maps with strides.I agree. One functionality I would expect from Tensors is to get "Sub-Tensors", e.g. for a (n)-Tensor fix m indexes and get a (n-m)-Tensor.OTOH, I don't think this is something that should hold up the merge, it's just a missing feature (no sub-tensor support yet anyway).

Maybe it would be best to leave the Blend functionality out for now and add it later to all of Eigen? (Or add it first, if it's something that can be done very soon and then merge this.)

Christoph -- ---------------------------------------------- Dipl.-Inf., Dipl.-Math. Christoph Hertzberg Cartesium 0.049 Universität Bremen Enrique-Schmidt-Straße 5 28359 Bremen Tel: +49 (421) 218-64252 ----------------------------------------------

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

**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:
**[eigen] Assignment of Complex Number** - 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/ |