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

 
 

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

Submitted by:  Charles Praplan <charprap>
Submitted on:  Thu 13 May 2021 07:56:50 AM UTC  
 
Category:  Octave Forge Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  None Assigned to:  None
Originator Name:  Open/Closed:  Open
Release:  dev Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

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>
Project Administrator
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>
Project Administrator
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 charprap (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    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 Forge 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.6