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

[ Thread Index | Date Index | More Archives ]


I did not read the entire pull-request, so I only have some sparse additions to the discussion. I hope my comments will not sound to much like "you did everything wrong" :) -- I generally appreciate the effort both of you took so far!

On 11.06.2014 01:31, Christian Seiler wrote:
But just for the future: next time could you perhaps tell me in advance
you're working on something? It's a good thing I only put together some
ideas so far but didn't actually start coding something yet, otherwise
we'd have duplicated a lot of work.

That is a good idea in general ... (I'll add a sentence to the "Contributing to Eigen" page on that).

That said, there are a lot of details which I think should be improved.
I personally took the approach of first writing stuff down in the wiki
and *then* wanting to implement it, because I wanted to get these kind
of details right in the first place. On the other hand, I have no code
to show here and you have implemented several thousand lines, so maybe
your approach of first doing something and then worrying about the
details is the better one. ;-)

I guess some parts would have benefited from being discussed first.
Especially, some additions you made which are not limited to Tensors (vectorized select expressions, GPU/CPU parallelization, ...) But I admit this is unfortunately a bad approach if you want to use the features immediately.

    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.

    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)

2. GPU / Threading support: So you probably implemented this here,
    because you needed it yourself. I'm all for it, and the
    EIGEN_DEVICE_FUNC additions definitely should go in (they're in
    Eigen/Core after all). And while I think it's really cool what you
    did there, with .device() and all that, I think this is something
    that affects all of Eigen and I wouldn't want to add this to the
    Tensor module without first having a more general discussion about

I agree. That functionality is not restricted to be usable for Tensors, so preferable we should discuss how to cleanly include this into Core.
The same goes for the vectorized select expressions.

3. General class structure:

    a. I don't like two classes TensorFixedSize and Tensor. I think
       there should be only one class, like Matrix or Vector.

I agree. To be consistent with Matrices, it would be nice to also support mixed Dynamic and fixed dimensions.

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.

    b. The HasBlend / pblend() stuff is probably also something that
       could benefit the rest of Eigen, right? OTOH, the for loop over
       the packet size seems potentially inefficient to me (at least on
       CPUs) - can't you just do a packet() on m_condImpl and store that
       in the selector?

Blend functionality will be required for the Core to efficiently support vector comparisons and most importantly to have an efficient Array<bool,...>::select(exprTrue, exprFalse) expression. For that purpose I doubt that struct {bool select[N];}; allows for efficient implementations. Here the idea of introducing an EigenBool<type> packet was proposed.

5. API stuff:

    a. "Tensor" class prefix. For the internal classes you obviously
       went the TensorXXX route, i.e. TensorEvaluator, TensorCwiseXXXOp
       etc. Since I raise this as a question in this thread, I thought
       about it some more and it's probably the easiest solution.

       That said, I really don't like names like TensorMap<> etc. that
       people will then use from the public API. Also, later on there's
       probably going to be something like Ref<> etc. I think it should
       be better to leverage the same names for the Tensor module, and I
       think I have an idea how to do that without making the rest of
       Eigen fall apart, but I have to try to see if it works first.

To be consistent with our current syntax, it should be
  Map<Tensor<...> > and Map<const Tensor<...> >
and the same for Ref<...>.

That all said, I think the best way to move forward is to merge your
changes sooner rather than later, otherwise it's going to be a huge
mess. I see two things that should definitely be handled from my
perspective before merging:

I mostly agree, concerning changes only affecting unsupported/
I don't like to to merge any substantial changes into Core without proper discussion (because changing these later on is likely to be an even bigger mess).

  * change the name Eigen::array to Eigen::std_array or something
    similar. The possibility of confusion w.r.t. Eigen::Array is too
    great in my eyes.

See my comment above: I don't see a general advantage of introducing array for things that Array can be re-used for.

After those two things I think the next step would be to merge the rest
of your stuff as-is for the moment being.

I think we also should find an agreement regarding C++11-emulation first. I don't think including CXX11 headers without proper C++11 support shall be possible.


Dipl.-Inf., Dipl.-Math. Christoph Hertzberg
Cartesium 0.049
Universität Bremen
Enrique-Schmidt-Straße 5
28359 Bremen

Tel: +49 (421) 218-64252

Mail converted by MHonArc 2.6.19+