[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: firls.m, part 2

From: Nicholas Jankowski
Subject: Re: firls.m, part 2
Date: Wed, 21 Jun 2017 14:27:04 -0400

On Wed, Jun 21, 2017 at 1:47 PM, Nicholas Jankowski <address@hidden> wrote:
> On Wed, Jun 21, 2017 at 1:25 PM, je suis <address@hidden> wrote:
>> Right, I hope I covered most of the possibilities, the script is
>> updated in I've also
>> modified/corrected other checks in there, I hope it's more robust.
>> Should I also add some examples? There are some in the description,
>> but should they also be added with the %! line beginning? And, if
>> all's well, should I use now?
>> Vlad
> trying your first example, I still get:
>>> h = firls(30, [0 0.3 0.4 1], [0 0 1 1])
> error: 'k2' undefined near line 367 column 19
> error: called from
>     firls at line 367 column 10

So, yes, I think you should put a few simple examples in your self tests.  It doesn't need to be exhaustive, but you want to make sure output doesn't break. The examples are fine for that. If there are simple, or even trivial, cases,  those may be easier to write simple tests for.

Generally for tests, I like to have both an error test to go with each input check and a testcase for any major input options or code branches. This is where you try to break your function on purpose.  For example, I see you have a narginchk and that's covered by your error tests, but a length(N) check that doesn't get tested by any error tests.  similarly, you don't have a test for non-integer N, nor any self-test.

>> firls (2.1, [0 1], [0 1])
warning: non-integer range used as index
warning: called from
    firls at line 290 column 3
error: q(2.1): subscripts must be either integers 1 to (2^31)-1 or logicals
error: called from
    firls at line 290 column 3

this isn't called until the call to toeplitz.

>> firls ('a', [0 1], [0 1])
error: mod: wrong type argument 'sq_string'

then, prior to any upload running a 'test firls' will let you know things are still behaving.

At this point I'd also recommend naming the file firls.m.  it is going to be used to replace firls and as long as you have it saved in your working directory Octave will always use that version first. And feel free to create a bug tracker entry. call out that the current firls is deficient in whatever ways, give a reference link to the mailing list conversations, a link to your github, etc. cc the package maintainer on the bug report (Mike), then when it gets to the point that the function is about ready to be submitted we can generate a patch to do so.

fixing any obvious bugs (k2?), checking output against examples & matlab, adding tests, and maybe revising a bit to match Octave's preferred 'coding style' [1], should about cover it.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]