commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thomas Neidhart <thomas.neidh...@gmail.com>
Subject Re: [Math] Accepting contributions (Was: svn commit: r1486982 - ...)
Date Wed, 29 May 2013 07:48:11 GMT
On Wed, May 29, 2013 at 12:40 AM, Gilles <gilles@harfang.homelinux.org>wrote:

> On Tue, 28 May 2013 20:47:30 +0200, Thomas Neidhart wrote:
>
>> On 05/28/2013 08:20 PM, Gilles wrote:
>>
>>> On Tue, 28 May 2013 16:04:32 -0000, tn@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).
>>
>
> My reply was based on reading the diff.
>
> [Are you really using capital letters as variable names?]


I thought it makes sense in this case (and did a grep before in the
codebase to see if there are other occurrences of it)


>
>  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<http://docs.scipy.org/doc/numpy/reference/generated/numpy.convolve.html>
>> )
>> so I guess there will be a practical use for it.
>>
>
> By this post, I ask whether it is sufficient reason for committing
> sub-par code that is currently not used by anyone.
> I suggested the OP 3 ways to start getting involved; the absence of
> reaction was IMHO reason enough not to rush including this code.
> [It has obviously nothing to do with judging that convolution is not
> worth implementing.]
>
> CM needs contributors, right; but it is quite unrelated to piling up
> unused/untested code.
> At one point, some years ago, there was a sort of acknowledgment that
> submitting a contribution was accompanied with an informal commitment
> to maintain it.
> Lacking that either gives more work to current committers or lowers
> the average quality of the library.
>

Well yes, I agree in general, but I did not expect to run into trouble with
this contribution: it's small and simple enough that everybody can dig into
it within minutes.
Also there are tests, and you can easily compare its result with other
implementations.

I think we do a pretty good job in maintaining the library, where we are
clearly lacking is documentation, and there I would like to see more
contributions from our users too if possible.

Thomas

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message