Re: [eigen] Eigen::FFT and Eigen::Complex in repo

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

awesome ! This is high quality code and a great addition to Eigen!
Below are the nitpicks:


FFT line 48:

template <typename _Scalar,
         typename _Traits=DEFAULT_FFT_IMPL<_Scalar>
         >

First, why call it Traits when Impl seems to be what it really is? It
collides with what we're calling traits elsewhere in Eigen: traits are
properties like some enums and typedefs, not a whole implementation.

Also, here you're passing _Scalar twice, and are using a macro. You
can improve on both fronts in 2 ways:
* either you let _Scalar be deduced from _Impl

template <typename _Impl>
class FFT
{
  typedef typename _Impl::Scalar Scalar;
// or if that doesn't work and you need forward-declaration, do as we
do elsewhere in Eigen:
  typedef typename ei_traits<_Impl>::Scalar Scalar;
// and specialize ei_traits for _Impl just before the definition of _Impl
};

* or you can keep the _Scalar parameter and let _Impl be a template
template parameter:

template <typename _Scalar, template<typename t> class _Impl>
class FFT
{
....
};


ei_fftw_impl.h line 25:

the using namespace Eigen is best centralized in FFT.

below, same file:
i'd be interested in a rationalization why none of these small
functions are marked inline. For this particular code, did you find
that compilers were already inlining enough spontaneously? If you have
deep chains of trivial functions calling each other, inline is really
needed, but i don't know if that's the case.

line 170:  float literal 1. needs to be rewritten Scalar(1) or else
you'll get warnings on certain compilers.



ei_kissfft_impl.h:

the includes are best centralized in FFT.

(i didn't mention that your filenames don't adhere to our conventions,
it's not hugely important, but anyway that would be KissFFT.h)

Need rationalization why you're using std::vector and not Eigen vectors.

(On the other hand, using plain C arrays / ptrs needs less of a
justification, since when applicable it compiles fast and to minimal
code. As long as using Eigen objects wouldn't allow vectorization or
unrolling.).

Data members are private while elsewhere in Eigen we default to
protected just in case someone might want to inherit, if only to
adjust API to ease porting, etc. Any particular reason for private?



Complex:

line 71:

    StandardComplex std_type() const {return StandardComplex(real(),imag());}
    StandardComplex & std_type() {return
*reinterpret_cast<StandardComplex*>(this);}

I don't understand. The 2nd line assumes that the data layout is
identical, between Complex and StandardComplex. So why not just
implement the const-qualified version in the same way, returning a
const reference? That would be much faster! Or if that would be unsafe
(if the data layout isn't guaranteed compatible --- don't remember
what we had concluded) then the non-const version is unsafe as it is.

line 108:

template <typename T>
T ei_to_std( const T & x) {return x;}

I don't understand why this is useful.


Thanks,
Benoit

2009/10/20 Mark Borgerding <mark@xxxxxxxxxxxxxx>:
> The FFT module has been pushed into the eigen2 repository.
>
> It currently lives in "unsupported".   The interface is still somewhat
> fluid.
>
> Also of interest is the unsupported/Complex class.
>
> Comments & code changes welcome.
>
>
> -- Mark
>
>
>