| Re: [eigen] [Review] Pull request 66, Huge Tensor module improvements |
[ Thread Index |
Date Index
| More lists.tuxfamily.org/eigen Archives
]
Hi,
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
it.
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 http://eigen.tuxfamily.org/bz/show_bug.cgi?id=97 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.
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
----------------------------------------------