bugGNU Octave - Bugs: bug #53205, [octave forge] (signal) buttord...

 
 

bug #53205: [octave forge] (signal) buttord function 's' option

Submitter:  Botond Sandor Kirei <botondkirei>
Submitted:  Thu 22 Feb 2018 01:17:16 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Duplicate Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 05 Jun 2018 10:47:23 PM UTC, comment #14: 

Ok, no activity here in a few weeks so I guess I'll just consider this item closed as a duplicate of bug #46444.

Mike Miller <mtmiller>
Group Member
Wed 02 May 2018 04:56:34 PM UTC, comment #13: 

I am talking about the 21 additional tests that you are proposing here, that are not included in the function file yet. I would appreciate it if you could run your tests against the latest buttord function and see where the failures are.

Mike Miller <mtmiller>
Group Member
Wed 02 May 2018 08:31:46 AM UTC, comment #12: 

I've checked out the default tree from Sourceforge of the signal package and run "test buttord". On my computer passes all the tests ("31 out of 31 test passed").

Later I will check the cheby1ord, cheby2ord and ellipord functions too.

The new Octave release (4.2.1) comes with the old Signal package (version 1.3.2) that does not contain the "s" option.

Botond Sandor Kirei <botondkirei>
Mon 30 Apr 2018 08:24:09 PM UTC, comment #11: 

A few of the tests (4/21) fail with the buttord improvements from bug #46444. I don't have time to look at the specifics of the failures at the moment, but you are welcome to compare the results, see whether a tolerance is needed in some of the tests, or if there is something that needs to be changed in the buttord and/or butter functions.

Mike Miller <mtmiller>
Group Member
Wed 11 Apr 2018 10:09:51 AM UTC, comment #10: 

I rewrote the verification of the results using the assert function. I left capital A and B, because they are the usual symbols denoting the nominator and the denominator of a transfer function.

you find the modifications in:
https://sourceforge.net/u/botondkirei/octave/ci/default/tree/inst/buttord.m

Botond Sandor Kirei <botondkirei>
Tue 10 Apr 2018 10:11:50 PM UTC, comment #9: 

In your %!test blocks, instead of this pattern


%! if ( (Mag(1) >= -3) && (Mag(2) <= -20))
%!   disp("Lowpass filter design test 1. PASSED - Wp=1, Ws=3, Rp=3, Rs=20");
%! else
%!   error("Lowpass filter design test 1. FAILED - Wp=1, Ws=3, Rp=3, Rs=20");
%! endif


you should use the assert function, and have something like this


%! assert (mag(1) >= -3)
%! assert (mag(2) <= -20)


If the assert fails, the test function will take care of showing the test block that failed. If you want the user to see explicitly that Wp=1 and Ws=3, etc, then maybe you should write the tests like this


%!test
%! Wp = 1; Ws = 3; Rp = 3; Rs = 20;
%! [n, Wc] = buttord (Wp, Ws, Rp, Rs, "s");
%! [b, a] = butter (n, Wc, "s");
%! mag = 20 * log10 (abs (freqs (b, a, [Wp, Ws])));
%! assert (mag(1) >= -Rp)
%! assert (mag(2) <= -Rs)


In this way, if the test fails, the user will see the entire code block including the local variable assignments, so it's all explicit. An added benefit is that it's explicit that the thing you are testing for is that the filter frequency response at particular frequencies Wp and Ws corresponds to the declared design parameters. This also adheres more closely to the test pattern used by most Octave functions. I've also fixed the coding style by adding whitespace, using double quotes, and lower-casing the 'b', 'a', and 'mag' variable names.

Mike Miller <mtmiller>
Group Member
Thu 05 Apr 2018 02:34:27 PM UTC, comment #8: 

Hello!

I've done the next corrections:
1. broke the test cases into sevaral smaller ones
2. warnings are thrown with the "warning" function
3. if statements are corrected to comply with octave coding style.
You may find the modifications here:
https://sourceforge.net/u/botondkirei/octave/ci/default/tree/inst/buttord.m
I'm not sure how to proceed with the print_usage() function to be at the beginning of the code.

I've took a look at bug #46444. I'm not used to work with .diff files, I'm not sure what and how is modified. Is the code saved into a fork where I can see the code as is (not as differences)?

Botond Sandor Kirei <botondkirei>
Tue 03 Apr 2018 08:06:56 PM UTC, comment #7: 

There is no deadline. There will be a new release of the signal package within the next couple weeks, with or without these fixes. I do hope to make more regular releases than in the past year, so it shouldn't be a big deal if this isn't fixed before the release.

I think the changes in bug #46444 are more substantial and useful, but I think the test cases you have written in your submission may be useful to merge in. So I am leaving this bug open as a reminder to pull the test cases in eventually.

If you have the time, can you look at flattening the test cases into individual %!test blocks as I suggested?

Mike Miller <mtmiller>
Group Member
Fri 23 Mar 2018 08:46:29 PM UTC, comment #6: 

Yes, I will take a look at the other bug, but it will take some time. Are you in a hurry with the review? Should I hurry?


Botond Sandor Kirei <botondkirei>
Tue 20 Mar 2018 10:32:30 PM UTC, comment #5: 

The work you have done so far to add this option to the buttord function looks very good. There has been some previous work done to add the analog option to all of the ord functions. Would you mind taking a look at bug #46444 to compare the work done there? I need to review all of this work and your work and come up with the best combined improvements.

Aside from merging your work with bug #46444 and basic functionality, some more coding style improvements would be very welcome in the revision that you have currently.

As a few examples

  • use "if (numel (Wp) == 1)" instead of "iff ( numel(Wp) == 1 )"
  • use "if (Wp(1) > Ws(1))" instead of "if Wp(1) > Ws(1)"
  • keep print_usage at the top of the function
  • don't use printf in the function, if you need to warn, use the warning function


You can also vastly simplify the test cases that you wrote at the end. Instead of making a loop over some test index, just write each as its own test block. The test function itself will take care of showing the code and the specifics of the test failure if necessary. There are plenty of examples of well written tests in Octave and in the signal package.

Mike Miller <mtmiller>
Group Member
Mon 26 Feb 2018 01:39:29 PM UTC, comment #4: 

Dear Mike,

Would you take a look to the corrected version of buttord function, before I try to merge it to the signal package?
Here is the link:
https://sourceforge.net/u/botondkirei/octave/ci/default/tree/inst/buttord.m

I revised to code to comply with Octave coding style, also added comments and provided several tests (low-, high-, pass- and stop-band).

If you say it's OK, I will try a merge. Pls, let me know if you have other suggestions.

Regards,
Botond

Botond Sandor Kirei <botondkirei>
Fri 23 Feb 2018 08:28:06 PM UTC, comment #3: 

Yes, I will adapt the code to Octave coding style. It will take a few days.

Botond Sandor Kirei <botondkirei>
Fri 23 Feb 2018 06:02:35 PM UTC, comment #2: 

Thanks for working on a fix to this missing feature. Would you mind working on it a bit more to adapt your code to Octave's coding style? https://wiki.octave.org/Octave_style_guide

Can you also add one or more test cases to the end of the function to verify that the new option works?

Mike Miller <mtmiller>
Group Member
Fri 23 Feb 2018 03:08:41 PM UTC, comment #1: 

Added package maintainers to CC list.

Olaf Till <i7tiol>
Group Member
Thu 22 Feb 2018 01:17:16 PM UTC, original submission:  

The MATALB function "buttord" is suitable to obtain the order of an analogic filter too. The common use for analog filter design is "buttord(...,'s')" -  the 's' flag is appended in the argument list.

The current versions of Siganl package of OCTAVE is not compliant with the MATLAB function.

Proposed solution:

I am not sure how to contribute to octave's signal package codebase, thus I created a development branch:
https://sourceforge.net/u/botondkirei/octave/ci/default/tree/

The proposed implementation is in the branches buttord.m function.

Botond Sandor Kirei <botondkirei>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Digest:
   bug dependencies.

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by botondkirei (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-06-05 mtmiller StatusPostponed Duplicate
        Open/ClosedOpen Closed
    2018-05-01 mtmiller Release4.2.1 dev
    2018-04-30 mtmiller StatusNone Postponed
    2018-04-04 mtmiller Dependencies- Depends on bugs #46444
    2018-02-23 mtmiller Carbon-CopyRemoved doug stewart <doug.dastew@gmail.com> -
    2018-02-23 mtmiller Carbon-CopyRemoved 77635 -
    2018-02-23 mtmiller Carbon-CopyRemoved alexander wilms <f.alexander.wilms@gmail.com> -
    2018-02-23 mtmiller Summary[Octave-Forge: signal pkg] buttord function MATLAB compatibility issue [octave forge] (signal) buttord function 's' option
    2018-02-23 i7tiol Carbon-Copy- Added doug stewart <doug.dastew@gmail.com>
        Carbon-Copy- Added alexander wilms <f.alexander.wilms@gmail.com>
    2018-02-23 rik5 Carbon-CopyRemoved 72865 -
    2018-02-23 rik5 CategoryOctave Function Octave Package
        Summarybuttord function MATLAB compatibility issue [Octave-Forge: signal pkg] buttord function MATLAB compatibility issue

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code