On 05/28/2013 08:20 PM, Gilles wrote:
> On Tue, 28 May 2013 16:04:32 -0000, t...@apache.org wrote:
>> Author: tn
>> Date: Tue May 28 16:04:32 2013
>> New Revision: 1486982
>>
>> URL: http://svn.apache.org/r1486982
>> Log:
>> [MATH-851] Added method MathArrays.convolve, thanks to Clemens Novak
>> for the patch.
>
> -1
>
> A significant part of this small patch (code and doc) contains formatting
> mistakes and non-conventional notations, as was indicated in the comments.
> If someone took the time to review something, please take it into account.
>
> The unit test is also not correctly defined; we agreed that newer code
> should test _one_ thing per test method.
> Also, precondition checks should use the "expected" attribute of the
> "@Test" annotation.
Most of the suggestions were already corrected by the OP with an updated
patch, the rest I changed to the usual style I use when committing (see
the commit log).
> Efficiency-wise, I wonder about the condition nested inside the
> double-loop:
>
> if ((j > -1) && (j < N) ) {
> yn = yn + x[j] * h[k];
> }
yes I agree, this could be further improved.
> As was also suggested in the comments, a "real" application (and/or a
> discussion on the "dev" ML, preferably initiated by the OP) would have
> been welcome to assess the pertinence of including this code in the
> proposed form.
It is also present in numpy (see
http://docs.scipy.org/doc/numpy/reference/generated/numpy.convolve.html)
so I guess there will be a practical use for it.
Thomas
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
