patchGNU Octave - Patches: patch #8078, [octave forge] (signal) ultrwin:...

 
 

patch #8078: [octave forge] (signal) ultrwin: new function, chebwin rebase

Submitter:  Rob Sykes <robsykes>
Submitted:  Sat 15 Jun 2013 07:32:04 PM UTC
   
 
Category:  Forge : new feature Priority:  5 - Normal
Status:  Postponed Privacy:  Public
Assigned to:  mtmiller Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Thu 23 Jan 2014 06:22:30 AM UTC, comment #3: 

I've committed most of your patch, with some changes to the .m and .cc files. The ultrwin function will be part of the next signal package release.

http://sourceforge.net/p/octave/signal/ci/2842bf

I also merged the C and C++ files together as I proposed, this should simplify maintenance and make changes easier to follow.

http://sourceforge.net/p/octave/signal/ci/4b88d8

I have not updated chebwin to use ultrwin yet, I may hold off on that until after the release that I want to get out soon. I'll leave this patch item open until that is done. That kind of change should be suitable for a followon bugfix release.

Thanks for your contribution!

Mike Miller <mtmiller>
Group Member
Wed 15 Jan 2014 08:03:03 PM UTC, comment #2: 


> * I prefer Octave coding standards for the signal package (# comments, endif,
> endfunction, and GNU style for spacing and indentation). I can clean these up
> easily enough when I incorporate your patch.


Hi Mike, certainly, this is appropriate for the .m & .cc files. 

> * Is there any reason not to combine ultrwin.c and ultrwin_.cc into one file?


Perhaps; ultrwin.[ch] can be used by any language that can wrap or link-to C, so I was hoping to point other projects at these files. (Being 'just a window', it didn't seem to warrant a dedicated repo somewhere). But if it's a problem, no worries, go ahead and combine; a 'cut here' comment can be fine.

> * We prefer to use leading and trailing double underscores for private
> functions, so the compiled function should be _ultrwin_ instead of ultrwin_


Yes, of course.

> * It would probably make it clearer to split this into two patches, one
> introducing the new function and one reimplementing chebwin based on ultrwin.


No problem with that.

> Please reply to discuss or if you want to post updated patches to this tracker
> item. In the mean time I'll test actually using the new function and make
> sure
> it works for me.


To compare the old/new chebwin, you could try something like:

tic; b=chebwin(1e7,300); toc; [h,f]=freqz(b,1,2^14); plot(f/pi, 20*log10(abs(h/h(1))))

Here, the new chebwin is faster and more accurate.

BTW, I've spotted a typo in the ultrwin.m documentation: "<= 1.5" should be "<= -1.5".

Also, if the name 'ultrwin' is too terse, please choose something that satisfies current Octave naming guidelines.

Rob Sykes <robsykes>
Wed 15 Jan 2014 04:00:03 AM UTC, comment #1: 

Hi Rob, I apologize for the long delay it took me to get around to reviewing your patch. There is a lot of code here to incorporate, I would like to compare both results and performance to make sure we're not losing anything against the current chebwin.

Just based on initial reading of the patch, here is some feedback:

  • I prefer Octave coding standards for the signal package (# comments, endif, endfunction, and GNU style for spacing and indentation). I can clean these up easily enough when I incorporate your patch.
  • Is there any reason not to combine ultrwin.c and ultrwin_.cc into one file? This would make compilation and maintenance easier.
  • We prefer to use leading and trailing double underscores for private functions, so the compiled function should be _ultrwin_ instead of ultrwin_ (sorry, savannah will turn one pair of underscores into italics).
  • It would probably make it clearer to split this into two patches, one introducing the new function and one reimplementing chebwin based on ultrwin.


Please reply to discuss or if you want to post updated patches to this tracker item. In the mean time I'll test actually using the new function and make sure it works for me.

Mike Miller <mtmiller>
Group Member
Sat 15 Jun 2013 07:32:04 PM UTC, original submission:  

Adds the Ultraspherical window function:

ultrwin.m

Also, reimplements:

chebwin.m

as a special case of the above.

Rob Sykes <robsykes>

 

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

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-03-07 mtmiller Carbon-CopyRemoved 80942 -
    2018-03-30 mtmiller CategoryNone Forge : new feature
        StatusIn Progress Postponed
        Summary[signal package new function] ultrwin.m [octave forge] (signal) ultrwin: new function, chebwin rebase
    2014-01-15 mtmiller StatusNone In Progress
    2013-10-18 mtmiller Assigned toNone mtmiller
    2013-06-15 robsykes Attached File- Added octave-signal-ultrwin.patch, #28322

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code