patchGNU Octave - Patches: patch #10351, (signal) firpm portability fixes

 
 

patch #10351: (signal) firpm portability fixes

Submitter:  Rob Sykes <robsykes>
Submitted:  Thu 18 May 2023 06:42:42 PM UTC
   
 
Category:  Forge : other Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  lostbard Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 20 May 2023 07:31:32 PM UTC, comment #8: 

I tested hg id 3f009c98fe11 tip @ against octave 8.2.1 on both x64-64 and aarch64 and it passes tests and demos run fine.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sat 20 May 2023 11:33:34 AM UTC, comment #7: 

i'm using octave 8.

I believe in octave 7 they started counting input arguments vs expected arguments

John Donoghue <lostbard>
Group Member
Sat 20 May 2023 07:46:33 AM UTC, comment #6: 

I’ve retested using Octave versions 5.2.0 & 6.2.0 and I don’t see any error.
However, the addition of the w argument per your suggestion also works here so I guess we should go with that.

Rob Sykes <robsykes>
Fri 19 May 2023 05:48:01 PM UTC, comment #5: 

I applied the patch, but there is still an error in one of the demos:

demo firpm 8
firpm example 8:
 cic = @(f) (sin (pi*(f+eps*!f)/2)./sin (pi*(f+eps*!f)/2/10)/10).^4;

 if compare_versions(OCTAVE_VERSION, '6', '<')
   eval('b = firpm (30, [0 .5 .7 1], @(n,f,g) deal (a=(g<=f(2))./cic (g), 1./(a+!a)));')
 else
   function [ag wg] = resp (n,f,g) ag = (g<=f(2))./cic (g); wg = 1./(ag+!ag); endfunction
   b = firpm (30, [0 .5 .7 1], @resp);
 endif

 clf; [h f]=freqz (b); plot (f/=pi, 20*log10 (abs (h)))
 grid on; axis ([0 1 -60 6]); set (gca, 'xtick', [0:.1:1])
 title (sprintf ('firpm type-I CIC-compensation filter (order=%i)', length (b) - 1));
 ylabel ('Magnitude response (dB)'); xlabel ('Frequency (normalized)')
 axes ('position', [2 3 4 3]/10)
 plot (f, 20*log10 (abs (h).*cic (f))); axis ([0 .55 -.04 .04]); grid on
 title ('Compensated filter response')
 %--------------------------------------------------
 % Figure shows transfer details of CIC-compensation
 % filter design.


firpm example 8: failed
resp: function called with too many inputs



I guessing it should be:


diff -r 5ca91861a7c9 src/firpm.cc
--- a/src/firpm.cc      Fri May 19 13:37:23 2023 -0400
+++ b/src/firpm.cc      Fri May 19 13:47:00 2023 -0400
@@ -1029,9 +1029,9 @@
 %! cic = @(f) (sin (pi*(f+eps*!f)/2)./sin (pi*(f+eps*!f)/2/10)/10).^4;
 %!
 %! if compare_versions(OCTAVE_VERSION, '6', '<')
-%!   eval('b = firpm (30, [0 .5 .7 1], @(n,f,g) deal (a=(g<=f(2))./cic (g), 1./(a+!a)));')
+%!   eval('b = firpm (30, [0 .5 .7 1], @(n,f,g, w) deal (a=(g<=f(2))./cic (g), 1./(a+!a)));')
 %! else
-%!   function [ag wg] = resp (n,f,g) ag = (g<=f(2))./cic (g); wg = 1./(ag+!ag); endfunction
+%!   function [ag wg] = resp (n,f,g,w) ag = (g<=f(2))./cic (g); wg = 1./(ag+!ag); endfunction
 %!   b = firpm (30, [0 .5 .7 1], @resp);
 %! endif
 %!


John Donoghue <lostbard>
Group Member
Fri 19 May 2023 03:55:28 PM UTC, comment #4: 

comment #2:

> Also looks like patch undoes the octave compatibility functions - any reason for that?


TBH, I can’t remember the thinking on that. This 'new' patch was actually created back in 2021 but with no sign of the original patch going anywhere I didn't post it and had forgotten about it until now.

By all means, if it’s still useful to have the compatibility functions then just skip that part of the patch.

Rob Sykes <robsykes>
Fri 19 May 2023 03:41:56 PM UTC, comment #3: 

comment #1:

> See also bug #64214
>
> Not sure if if this fixes that as well ?
>
>

Yes, it looks like the same problem I saw when trying it on Octave 6 (the code was originally developed using Octave 5).

Rob Sykes <robsykes>
Fri 19 May 2023 12:58:07 PM UTC, comment #2: 

Also looks like patch undoes the octave compatibility functions - any reason for that?

John Donoghue <lostbard>
Group Member
Fri 19 May 2023 12:52:43 PM UTC, comment #1: 

See also bug #64214

Not sure if if this fixes that as well ?


John Donoghue <lostbard>
Group Member
Thu 18 May 2023 06:42:42 PM UTC, original submission:  

Fixes:

  •  test firpm    (adds ARM architecture compatibility)
  •  demo firpm    (adds compatibility with Octave V6)
  •  source-code whitespace errors
Rob Sykes <robsykes>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #54761:  firpm.patch.gz added by robsykes (9KiB - application/gzip)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dasergatskov (Posted a comment)
  • -email is unavailable- added by lostbard (Posted a comment)
  • -email is unavailable- added by robsykes (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 logged-in users can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-05-20 lostbard StatusNone Done
        Assigned toNone lostbard
        Open/ClosedOpen Closed
    2023-05-18 robsykes Attached File- Added firpm.patch.gz, #54761

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code