patchGNU Octave - Patches: patch #8078, [signal package new function]...

 
 

patch #8078: [signal package new function] ultrwin.m

Submitted by:  Rob Sykes <robsykes>
Submitted on:  Sat 15 Jun 2013 07:32:04 PM UTC  
 
Category: NonePriority: 5 - Normal
Status: In ProgressPrivacy: Public
Assigned to: Mike Miller <mtmiller>Open/Closed: Open

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

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

 

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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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 File(s):
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by mtmiller (Updated the item)
  • -unavailable- added by robsykes (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 15 Jan 2014 04:00:03 AM UTCmtmillerStatusNone=>In Progress
    Fri 18 Oct 2013 02:55:42 AM UTCmtmillerAssigned toNone=>mtmiller
    Sat 15 Jun 2013 07:32:04 PM UTCrobsykesAttached File-=>Added octave-signal-ultrwin.patch, #28322

    Back to the top


    Powered by Savane 3.1-cleanup