bugGNU Octave - Bugs: bug #60597, [octave forge] (signal) Bug in...

 
 

bug #60597: [octave forge] (signal) Bug in functions handling SOS

Submitter:  Charles Praplan <charprap>
Submitted:  Thu 13 May 2021 07:56:50 AM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  lostbard
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

Fri 28 Oct 2022 12:02:26 PM UTC, comment #13: 


Signal 1.4.3 released - closing as fixed

John Donoghue <lostbard>
Group Member
Wed 26 Oct 2022 07:18:02 PM UTC, comment #12: 

lostbard FYI the other bug charprap refers to in the original comment is bug #51936

When the new version of signal is released both of these bugs could be closed IIUC.

Anonymous
Tue 25 Oct 2022 08:50:55 PM UTC, comment #11: 

Assuming @mtmiller (current maintainer) has no issues with that.

John Donoghue <lostbard>
Group Member
Tue 25 Oct 2022 08:48:01 PM UTC, comment #10: 

I will probally see about putting out another release since I did the last one

John Donoghue <lostbard>
Group Member
Wed 14 Sep 2022 12:37:08 PM UTC, comment #9: 

Yeah - I'm not the official maintainer, but try to get other packages in a working state for mxe (and just in general) when I can after working on mxe and my own packages :)

For signal if I recall correctly, there were a couple of bugs that were stopping it install that I applied, and whilst at it grabbed a few of the other bugs and patches that were around that looked like low hanging fruit - I guess I missed this one.

I've applied it to the repo now so it will appear in the next release.

Thanks for the contributions and sorry for the long wait!


John Donoghue <lostbard>
Group Member
Sun 12 Jun 2022 09:40:35 AM UTC, comment #8: 

If it is any consolation:
Your contributions were explicitly mentioned in the developer meeting in April.

https://wiki.octave.org/Online_Developer_Meeting_(2022-04-26)

John Donoghue added some improvements and released signal v1.4.2.
However, I do not think John is the maintainer of the package.
Hopefully, John can provide more details.

Anonymous
Sun 12 Jun 2022 08:45:36 AM UTC, comment #7: 

Octave 7.1.0 for window has been delivered with signal-1.4.1.

I just installed signal-1.4.2 which has been released on 2022-04-22. The fix is not included in version 1.4.2.

If I recall correctly, signal-1.4.1 has been released in April 2019, I hope next release will take less time.

Charles Praplan <charprap>
Sat 11 Jun 2022 10:42:03 PM UTC, comment #6: 

Apologies for this oversight. This sometimes happens, inadvertently.
BTW, signal is at 1.4.2 now, did you check if the fix is included there now, of did you mix up release numbers?

I wasn't involved in the release process, but what I guess is that the package maintainer hasn't been active lately and the new release was made by the mxe-octave (Windows) maintainers to be able to include it with a new Windows installer that required other signal package fixes to avoid crashes and hard errors.
That mxe-octave maintainers had to do a lot of other work for that new Windows installer so I'm not surprised if they didn't hunt for more work, rather did the minimum to get the signal working again with newer Octave, and that way concentrated on getting forward with their actual project, i.e. the mxe-octave Windows installer.

Again, sorry that your contribution got ignored. Hopefully it'll be picked up and pushed soon.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 11 Jun 2022 08:37:42 AM UTC, comment #5: 

Hi,

I'm a little bit disappointed that the proposed correction of this bug:

 tf2sos([0 0 1], [1 1 1])
  ans =
     1.0000        0        0   1.0000   1.0000   1.0000

has not been implemented in the latest release 1.4.1 of the signal processing toolbox.

The correction and different improvements are available in this bug report since about 1 year!

Has this been forgotten or is there another good reason?

Charles Praplan <charprap>
Mon 24 May 2021 01:36:05 PM UTC, comment #4: 

Here are the 3 patches:
Finally for the function zp2sos.m, I decided not to combine real poles or zeros.
If this should be modified, simply change the value of DoNotCombineReal to zero in the function.
Please give me a feed-back, if the functions are ok for you.

(file #51468, file #51469, file #51470)

Charles Praplan <charprap>
Fri 14 May 2021 12:24:43 PM UTC, comment #3: 

No problem, I will continue editing the functions and post a new bug report for cplxreal.

Now about the Matlab compatibility :

Return values :
---------------
I'm pretty sure that these functions return correct results what is not the case of Matlab. See following example for which Matlab returs an incorrect value for A2:
B0=[1 0 0 1], A0=[1 1 1], sys=tf(B0, A0), [B1,A1]=tfdata(sys,'vector'), sos=tf2sos(B0,A0), [B2,A2]=sos2tf(sos)

Input arguments :
---------------
If it is clear that zp2sos must combine complex-conjugate poles or zeros, what to do with real poles or zeros?
My idea was to let the user decide, therefore the additional parameter for zp2sos . Now if Matlab compatibility is more important, we have to decide. Perhaps not combining real values is preferable (so that it is clear, seeing the result, that the poles or zeros are real). However the case of having multiple real zeros or poles is less frequent, so not very important.

Which solution do you prefer?


Charles Praplan <charprap>
Thu 13 May 2021 08:25:53 PM UTC, comment #2: 

I also think it would help to file a separate bug report for the cplxreal bug and fix that in its own change, since that is easily described and tested separately from these other larger filter SOS fixes.

Mike Miller <mtmiller>
Group Member
Thu 13 May 2021 06:13:21 PM UTC, comment #1: 

Your fixes seem mostly fine to me.

Matlab compatibility is important in the arguments and the return values, but it's not clear to me whether you are proposing being intentionally incompatible or just not sure.

Are you able and willing to continue editing these functions to make the formatting appropriate for Octave and maybe add some test cases?

Mike Miller <mtmiller>
Group Member
Thu 13 May 2021 07:56:50 AM UTC, original submission:  

Hi,

I already posted this under an old bug report (2017), but I think it has not been seen, since nobody replied...

I have encountered a problem with the function tf2sos which delivered a wrong result:

  tf2sos([0 0 1], [1 1 1])
  ans =
     1.0000        0        0   1.0000   1.0000   1.0000
 

which is obviously different from starting point. 

Having a look in savannah, I found this old bug report, handling the same topic.

So I worked a little bit on these functions handling second order sections, and I propose
some corrections (4 m-files appended: sos.7z)
The main idea behind the changes is to make the functions more general to handle polynomial
coefficients, zeros and poles regardless which transfer function they represent.
So the modified functions can handle analog and digital filer.
 

tf2sos     : tf2sos is ok but calls zp2sos which does not work correctly.

zp2sos     : zp2sos assumes that all 2nd-order polynomials are monic.
             This is a restriction, which does not even have a physical reason.
             If the assumption is not met, the function does not issue an error,
             but returns a wrong result.
       

             The function has been rewritten to overcome the limitation.
             Furthermore, a flag allows not to combine real poles or zeros.
       

             However in order to get correct results in all cases, a correction is
             needed in the function cplxreal as described below.
       

cplxreal.m : at line 70, real zeroes or poles with value 0 were handled as complex.

             The correction corrects this.

sos2tf     : sos2tf removes trailing zeros independently in numerator and denominator.
             This is not correct (even if Matlab does the same).
       

             The new version only removes trailing zeros which are present both in
             numerator and in denominator.
             The new version also removes leading zeros in the same way.
      

             My opinion is that the functions which return numerator and denominator
             should return vectors of the same size (even if Matlab does not).
             A good practice for the user is to always apply this rule.
       

sos2zp     : sos2zp considers the gain as the product of the b0 coefficients and
             issues an error if it is zero.
             This means that all b0s must be non-zero. This is a restriction which
             has no reason to exist.
       

             In the new version, the gain is calculated as the product of the first
             non-zero coefficients of the numerators.

I hope this will be useful and will be integrated in next release.

Charles Praplan <charprap>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51468:  zp2sos.patch added by charprap (11KiB - application/octet-stream)
file #51469:  sos2tf.patch added by charprap (9KiB - application/octet-stream)
file #51470:  sos2zp.patch added by charprap (9KiB - application/octet-stream)
file #51429:  sos.7z added by charprap (4KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by lostbard
  • -email is unavailable- added by lostbard (Posted a comment)
  • -email is unavailable- added by charprap (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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-10-28 lostbard StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2022-10-25 lostbard Carbon-Copy- Added mtmiller
    2022-09-14 lostbard StatusNone Ready For Test
        Assigned toNone lostbard
    2021-05-24 charprap Attached File- Added zp2sos.patch, #51468
        Attached File- Added sos2tf.patch, #51469
        Attached File- Added sos2zp.patch, #51470
    2021-05-13 mtmiller CategoryNone Octave Package
        Release6.2.0 dev
    2021-05-13 mmuetzel Carbon-CopyRemoved 102357 -
    2021-05-13 mmuetzel SummaryBug in functions handling SOS [octave forge] (signal) Bug in functions handling SOS
    2021-05-13 charprap Attached File- Added sos.7z, #51429

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code