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

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

*To*: eigen@xxxxxxxxxxxxxxxxxxx*Subject*: Re: [eigen] Eigen::FFT and Eigen::Complex in repo*From*: Benoit Jacob <jacob.benoit.1@xxxxxxxxx>*Date*: Tue, 20 Oct 2009 22:34:30 -0400*Dkim-signature*: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:content-type :content-transfer-encoding; bh=Q7LsqogdKs/qwPgZaI7KGReGpFea/LMCpgw/8S9UKA4=; b=In0v4wXkskMCm/yPhdshae383ekIzgO2XQwcwwzPOv0oqnl1Qs6fcONX3max+u1Ef9 LyqWxQ4eVB4Eq/OHHFvPoBnnc4PSgAskK6IQ0LFxZcj9h0jrMrbh6RLVkM9/SjUSeb7X f8bcn9UojOKBQewhpi4Pd90cLLKYuuwq/bxKU=*Domainkey-signature*: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type:content-transfer-encoding; b=uyBqMSoS6Mw2F2CJYkc4a9/oeJbqJszauPFtv/Q2gVu2t7JzP1wZRQlTGA0IEiZ8km 83WcNmZUu8W+hD3/kbMpy+5SCEydoM8yumxRtEZ1aY6h9YUmEmwBy7w0B3REStzkpgW5 Leo2GRO/nYwi0C9WxwFlQKP2yL/DpSsk/R/O4=

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 > > >

**Follow-Ups**:**Re: [eigen] Eigen::FFT and Eigen::Complex in repo***From:*Mark Borgerding

**Re: [eigen] Eigen::FFT and Eigen::Complex in repo***From:*Mark Borgerding

**References**:**[eigen] Eigen::FFT and Eigen::Complex in repo***From:*Mark Borgerding

**Messages sorted by:**[ date | thread ]- Prev by Date:
**[eigen] Eigen::FFT and Eigen::Complex in repo** - Next by Date:
**Re: [eigen] Eigen::FFT and Eigen::Complex in repo** - Previous by thread:
**[eigen] Eigen::FFT and Eigen::Complex in repo** - Next by thread:
**Re: [eigen] Eigen::FFT and Eigen::Complex in repo**

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