patchGNU Octave - Patches: patch #8379, signal package: Implemented...

 
 

patch #8379: signal package: Implemented uencode function

Submitter:  Ouzounis Georgios <gouzouni>
Submitted:  Wed 05 Mar 2014 10:51:57 PM UTC
   
 
Category:  Forge : new function Priority:  3 - Low
Status:  Done Privacy:  Public
Assigned to:  mtmiller Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 30 Mar 2018 01:59:16 AM UTC, comment #8: 

This function has finally been merged into the signal package repository. Sorry for the long delay.

https://hg.mtmxr.com/octave-signal/rev/26868f38b5cd

I made some additional fixups to this patch

https://hg.mtmxr.com/octave-signal/rev/e09c89596a6b

This will be included in signal 1.4.0. Thank you for your contribution!

Mike Miller <mtmiller>
Group Member
Tue 01 Aug 2017 02:42:50 PM UTC, comment #7: 

Is there anything I can do to help getting this into Octave?

Anonymous
Fri 07 Mar 2014 02:32:42 PM UTC, comment #6: 

Excellent, this is looking really great. Thanks for your patch. The coding style and changeset formatting look really good. I have not actually tested the code yet, but assuming you did and it works as expected because of your unit tests.

I can take this as is, but I will rewrite the commit message, please see http://wiki.octave.org/Commit_message_guidelines for some guidelines about how to write commit messages for Octave. Also look at the commit log on Octave or the signal repository for some examples of how to write it for your future submissions.

Thanks for your contribution!

Mike Miller <mtmiller>
Group Member
Fri 07 Mar 2014 12:35:14 PM UTC, comment #5: 

I have added a new patch making the required changes. These changes are :

- correct spacing to defferentiate between indexing and function calling
- use "fix()" instead of "isinteger()" to check is a number is an integer
- use '^' instead of '**' for exponentiation
- use "all()" to check the sign of entries in the input vector
- use symbol '&' for logical "and"
- use temporary logical index array when using the same complex indexing for than one.

It is defenitely more readable now! If there is anything else that must be done please let me know.

(file #30815)

Ouzounis Georgios <gouzouni>
Fri 07 Mar 2014 06:39:36 AM UTC, comment #4: 

Great work, you are definitely picking up the coding style and generating a proper mercurial changeset here.

A couple more nits to pick on this latest patch.

  • We try to differentiate between indexing (no space between variable and parenthesis) and function calls (with space).
  • If args must be integer-valued, then test for that, something like "n < 2 || n > 32 || n != fix (n)".
  • Use ^ instead of double-asterisk for exponentiation, both do work but for consistency it's better to stick to one operator.
  • Instead of "(in < 0) == zeros (size (in))", something like "all (in >= 0)" should be more efficient.
  • Instead of "logical ((in > -v) .* (in < v))", try "((in > -v) & (in < v))". Also when you're going to use the same such complex indexing more than once, you can use a temporary logical index array, something like



idx = (in > -v) & (in < v);
a(idx) = somefunc (a(idx));


I hope you can see I'm just focusing on little details now, overall this is a really good patch and I could definitely apply it to the signal repository as it is with only minor cleanup. I saw you're applying for GSoC so this seems like a good opportunity to absorb some of the common patterns and styles we use across the Octave code base.

Mike Miller <mtmiller>
Group Member
Thu 06 Mar 2014 05:32:01 PM UTC, comment #3: 

I have implemented the udecode function and I submit a new patch with both udecode and uencode and the required changes. Concretely,

-Textinfo docstrings have been wrapped up at 80 columns or less
-Author and Created comments have been removed
-Nargin check has become the first check to occur when the function is called
-Error messages have been rewritten
-INDEX file has been updated
-The functions have been rewritten without any for loops.

Please let me know if there are other things that are wrong and must be corrected.

(file #30805)

Ouzounis Georgios <gouzouni>
Thu 06 Mar 2014 12:27:34 AM UTC, comment #2: 

Hi Georgios, thanks for the patch. Since this function is intended for the signal package, I'm going to retitle it as such, to help me organize.

First, since this is intended to be in the signal package, could you redo this as a changeset on the signal repository? The Octave Forge packages are maintained separately from Octave itself, see http://sf.net/p/octave/signal for the appropriate hg repository. Make sure you add new functions to the appropriate section of the INDEX file when working on Forge packages.

Would you be interested in working on the udecode function as part of this same contribution? It would be nice to get both functions working together in one changeset, if you're interested in taking that on.

The coding style looks mostly correct. Some suggestions:

  • Please wrap the Texinfo docstrings at 80 columns or less
  • No need for Author and Created comments, version control tells us all we need to know
  • The nargin check should be the very first check, otherwise range checking on n could fail
  • Error messages should use the argument name, e.g. error ("func: N is not an integer")
  • Rewrite without using a for loop?


The last suggestion is a very good idea to get used to the style of programming with Octave and other environments where vectors and matrices are the primary data type. You should be able to rewrite the quantization part of this function without using a for loop and loop index variable. There are many examples in Octave and the signal package to draw from, feel free to ask on the mailing list if you need help.

Mike Miller <mtmiller>
Group Member
Wed 05 Mar 2014 10:55:29 PM UTC, comment #1: 

I implemented the uencode function of the signal package. Since it is my first time using Mercurial and submitting a patch I would appreciate any comments on it as a feedback for me to improve myself!

Ouzounis Georgios <gouzouni>
Wed 05 Mar 2014 10:51:57 PM UTC, original submission:  


Ouzounis Georgios <gouzouni>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #30815:  uencudec2.patch added by gouzouni (7KiB - text/x-patch - Patch redone 2)
file #30805:  uencudec.patch added by gouzouni (7KiB - text/x-patch - Patch redone)
file #30785:  uencodepatch.patch added by gouzouni (3KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by gouzouni (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-03-30 mtmiller CategoryNone Forge : new function
        StatusIn Progress Done
        Open/ClosedOpen Closed
    2014-03-07 mtmiller StatusNeed Info In Progress
    2014-03-07 gouzouni Attached File- Added uencudec2.patch, #30815
    2014-03-06 gouzouni Attached File- Added uencudec.patch, #30805
    2014-03-06 mtmiller Priority5 - Normal 3 - Low
        StatusNone Need Info
        Assigned toNone mtmiller
        SummaryImplemented uencode function in signal package. signal package: Implemented uencode function
    2014-03-05 gouzouni Attached File- Added uencodepatch.patch, #30785

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code