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

 
 

patch #8379: signal package: Implemented uencode function

Submitted by:  Ouzounis Georgios <gouzouni>
Submitted on:  Wed 05 Mar 2014 10:51:57 PM UTC  
 
Category: NonePriority: 3 - Low
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.

 

(Jump to the original submission Jump to the original submission)

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>
Project MemberIn charge of this item.
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

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

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 07 Mar 2014 02:32:42 PM UTCmtmillerStatusNeed Info=>In Progress
    Fri 07 Mar 2014 12:35:14 PM UTCgouzouniAttached File-=>Added uencudec2.patch, #30815
    Thu 06 Mar 2014 05:32:01 PM UTCgouzouniAttached File-=>Added uencudec.patch, #30805
    Thu 06 Mar 2014 12:27:34 AM UTCmtmillerPriority5 - Normal=>3 - Low
      StatusNone=>Need Info
      Assigned toNone=>mtmiller
      SummaryImplemented uencode function in signal package.=>signal package: Implemented uencode function
    Wed 05 Mar 2014 10:51:57 PM UTCgouzouniAttached File-=>Added uencodepatch.patch, #30785

    Back to the top


    Powered by Savane 3.1-cleanup