patchGNU Octave - Patches: patch #8575, [octave forge] (signal) lpc: new...

 
 

patch #8575: [octave forge] (signal) lpc: new function

Submitted by:  None
Submitted on:  Tue 18 Nov 2014 08:36:38 PM UTC  
 
Category:  Forge : new function Priority:  5 - Normal
Status:  Need Info Privacy:  Public
Assigned to:  Mike Miller <mtmiller> Originator Email:  -email is unavailable-
Open/Closed:  Open

Add a New Comment (Rich Markup)
   

You are not logged in

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

 

( Jump to the original submission)

Tue 10 Mar 2020 10:28:53 PM UTC, comment #8: 

Keep in mind that the signal package, like most Octave Forge packages, is community maintained, anyone is welcome to review patches and contributions. But we are all volunteers and do what we can, and so far I am the only one who has commented here.

I spent about 15 minutes and took a quick look at the latest submission here, and it seems mostly right, but I can see a few improvements that I would like to have made before committing it.

1. All strings should be double quoted, except where excessive backslashes make it unreadable. So the "xlabel" in the demo is ok, but the "xcorr" call should use double quotes.

2. The "@seealso" markup is lost because it comes after "@end deftypefn". Use the "@dots{}" Texinfo macro instead of a literal "..." when eliding an array or list.

3. Please delete all trailing whitespace. You can probably configure your editor to do this automatically, or do a global search and replace.

4. Demos shouldn't create new figures, they should use the implied figure created by the demo function. Multiple plots should either be subplots or should be made into separate demos.

5. In the second demo you have a long sprintf line, that should have a space before the first paren, and the long line should be split with "..." and string concatenation, no backslashes.

6. Instead of "[m, n] = size (...", use "n = columns (...", since the "m" variable is never used.

7. I think you should rearrange the order of the "@deftypefn/x" lines. In Octave, we typically start with the simplest calling form, and then show increasingly complex syntaxes with more arguments and return values. The first line should be the one that takes one argument and returns one value, followed by two arguments, followed by a line like "{[@var{a}, @var{g}] =} lpc (@dots{})" to show that either of the above can be used with two return values. See for example the docstring of the "fopen" function.

Other than these relatively small issues, I think this is an excellent submission, and with those changes can be added to the signal package. It would be even better if you could take the time to use Mercurial to create a proper changeset that can be imported to add this function and the appropriate NEWS file entry.

I can take care of all of these things given enough time, energy, and interest, but the more that you or anyone else can help out, the easier this can be added to the package.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Tue 10 Mar 2020 05:16:28 PM UTC, comment #7: 

I've noticed the function has not been added to the package, after 6 years. Does it need any further correction?

The last comment was about Matlab compatibility and coding style.

Well, I've noticed the functions on the packaged use "endif", "endfor", etc., which are not Matlab compatible. So I don't know what else could be incompatible.

About the coding style, I don't know what exactly is out of style.

Leonardo <leolca>
Mon 16 Apr 2018 08:17:30 PM UTC, comment #6: 

I've just added a missing assertion in test.

(file #43955)

Leonardo <leolca>
Fri 13 Apr 2018 09:14:59 PM UTC, comment #5: 

I've made some corrections and improvements.

(file #43921)

Leonardo <leolca>
Thu 12 Apr 2018 05:10:06 PM UTC, comment #4: 

Thank you for the changes. There is still more work to be done to make this Matlab compatible and to conform to our coding style.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Thu 05 Apr 2018 07:28:55 PM UTC, comment #3: 

Thanks for the feedback!
I've made some corrections and improvements. Two demos were added. One of them requires a sample audio file provided by ltfat package. I hope it is okay.

(file #43813)

Leonardo <leolca>
Wed 04 Apr 2018 06:03:19 AM UTC, comment #2: 

Thank you for your work on this lpc function file. I'm sorry it has taken so long for me to get around to looking at this submission.

The lpc function is definitely needed in the signal package, and your function seems basically functionally correct to me. There are some Matlab compatibility issues remaining, and some additional error checking that could be added to ensure x and p are correct. For example, Matlab lpc can be called with one argument, and p defaults to length(x)-1. The function should also verify that x is real. The value p can be equal to length(x). And the prediction error second return value should be a scalar, not a vector result.

This is definitely a good candidate function for including a demo that will be shown in the online manual. For example a demo that creates an autoregressive process and shows the original signal against the estimate, similar to what is shown on Matlab's help page.

And beyond that there are some small Octave coding style changes to make (use spaces before parentheses, but not when indexing; use '!' instead of '~'; use the name of the variable uppercased instead of 'arg 1' in error messages).

Again, sorry for the delay, but let me know if you are interested and motivated to work on this a bit more and submit another revision.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Tue 18 Nov 2014 11:54:25 PM UTC, comment #1: 

Thanks for your contribution. As we discussed over email, assigning myself to review and merge this into the signal package.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Tue 18 Nov 2014 08:36:38 PM UTC, original submission:  

A small contribution to octave-signal follows in the forked repository
https://bitbucket.org/leolca/octave-signal
or also attached here.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #43955:  lpc.m added by leolca (4KiB - text/x-objcsrc)
file #43954:  lpc.m added by leolca (4KiB - text/x-objcsrc - just added a missing assert in test)
file #43921:  lpc.m added by leolca (4KiB - text/x-objcsrc)
file #43813:  lpc.m added by leolca (4KiB - text/x-objcsrc)
file #32468:  lpc.m added by None (2KiB - text/x-objcsrc)

 

Depends on the following items: None found

Digest:
   patch dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by leolca (Updated the item)
  • -email is unavailable- added by None (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 logged-in users can vote.

     

     

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-03-07 mtmiller Carbon-CopyRemoved 80942 => -
    2018-04-16 leolca Attached File- => Added lpc.m, #43955
    2018-04-16 leolca Attached File- => Added lpc.m, #43954
    2018-04-13 leolca Attached File- => Added lpc.m, #43921
    2018-04-05 leolca Attached File- => Added lpc.m, #43813
    2018-04-04 mtmiller StatusNone => Need Info
    2018-03-30 mtmiller CategoryNone => Forge : new function
        Summarysignal package: lpc - linear predictor => [octave forge] (signal) lpc: new function
    2016-04-14 mtmiller Dependencies- => patch #8980 is dependent
    2014-11-18 mtmiller Assigned toNone => mtmiller
        Summarylinear predictor => signal package: lpc - linear predictor
    2014-11-18 mtmiller Dependencies- => patch #8574 is dependent
    2014-11-18 None Attached File- => Added lpc.m, #32468

    Back to the top


    Powered by Savane 3.5