[Octave-bug-tracker] [bug #51310] [octave forge] (signal) firls.m modification to include all 4 FIR types, Hilbert transformer, and differentiator

[Mike Miller]

Follow-up Comment #16, bug #51310 (project octave):

We are obviously not communicating very well, so I will stick to specifics.
Here are some specific technical feedbacks on the latest revision of this file
that I would like to fix before incorporating it into the signal package.

----

The following are what I referred to as "issues with the doc string":

* The doc string emits this error message from makeinfo, probably because of a
duplicate deftypefn somewhere.


>> help firls
/tmp/octave-help-QpBAq9:238: @bye seen before @end deftypefn


* Strings options in the doc string should be written as @qcode{"d"} instead
of 'd'. Strings in the examples should be written "d".

* The doc string should not mention technical details like the changelog or
that it requires a specific expint.m function.

* The doc string should use @example markup for code examples, see other
Octave functions for comparison.

* The examples in the doc string should probably use 'b' for the return value
to be consistent with the @deftypefn syntax.

* Please use the @cite markup in the doc string for referencing a paper or
book, see the Octave function lscov.m for an example using this.

----

The following are what I referred to as "the error reporting":

* Error messages should be formatted according to the Octave conventions,
instead of this


>> b = firls (-1, [0, 0.3, 0.4, 1], [1, 1, 0, 0])
error: The order (N) must be positive definite.


should be something like this


error: firls: N must be a positive integer


Every error message should start with "firls:" and the message should start
with a lowercase letter, unless it is the name of a parameter. The error
message should not end in a full stop/period. As another example, instead of
this


>> b = firls (31, [0, 0.3, 0.4, 1], [1, 1, 0])
error: The sizes of the frequency and magnitude vectors must be equal.


should be something like


error: firls: A and F must be vectors of equal size


* Warning messages should be raised using the warning function, not sprintf.
Even better, warning with a warning ID argument:


  warning ("signal:firls-order-incremented",
           "Type II and IV FIRs can't have non-zero amplitude at Nyquist; N
has been incremented");


----

The following are what I referred to as "unit test failures"


>> test firls.m
...
!!!!! test failed
ASSERT errors for:  assert (x,h,1e-15)

  Location  |  Observed  |  Expected  |  Reason
    (11)      -0.0073477   -0.0073477    Abs err 1.2299e-15 exceeds tol 1e-15
by 2e-16
    (13)      0.0011742    0.0011742     Abs err 1.9806e-15 exceeds tol 1e-15
by 1e-15
    (16)      -0.0066228   -0.0066228    Abs err 2.2647e-15 exceeds tol 1e-15
by 1e-15
    (17)      -0.011871    -0.011871     Abs err 1.2646e-15 exceeds tol 1e-15
by 3e-16
    (18)      0.0011213    0.0011213     Abs err 2.1053e-15 exceeds tol 1e-15
by 1e-15
...


Yes, these tests should pass. I think you are saying that the tests fail
because the "expected" data came from another source, like Matlab or SciPy,
and your algorithm produces different values that you consider more correct,
is that right? If you think that was clear, sorry, I have a lot going on and I
either forgot or read past that before. I think you think that I care about
matching Matlab precisely, but I don't, I just want to see the tests pass.
Either the "expected" values should be fudged or the tolerance should be
loosened to allow them to pass by default.

Unit tests should also use "d" and "h" instead of 'd' and 'h'.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?51310>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/