On Wed, May 29, 2013 at 12:40 AM, Gilles <gil...@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, 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). >> > > 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
