patchGNU Octave - Patches: patch #9449, image package: new function...

 
 

patch #9449: image package: new function viscircles.m

Submitter:  Alexander Wilms <alexanderw>
Submitted:  Thu 07 Sep 2017 08:55:53 AM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 11 Jan 2018 07:15:56 PM UTC, comment #6: 

The implementation has three issues:

  • the input checking of param-value pairs should be done with inputParser to simplify code.
  • it loops over each circle to draw which should be vectorized away
  • if the figure was on "hold" before the function, it will change its state (should use unwind_protect and restore the initial state)
  • returns an empty hg group instead of a group of the circles


I had an unfinished implementation of viscircles from 2015

https://bitbucket.org/carandraug/octave-image/commits/4e7cdf90c76ea3046432c529ee8e2a69781390b2

At the time, there was no option Color and EnhancedVisibility, there was EdgeColor and DrawBackgroundCircle. It seems like Matlab just maps the old property names to the new ones so I did the same. This makes the input checking a bit messier but a lot of the existing code snippets make use of the old names so I think we should keep them.

I amended my previous code with those changes and pushed it to the default branch:

http://hg.code.sf.net/p/octave/image/rev/5a0b96c36f41

and added the demo and tests from this patch

http://hg.code.sf.net/p/octave/image/rev/aa5a1ca19146

The returned hg group is different from Matlab but I think the issue is how line and plot works and not on the viscircles code. I'm not great at the whole potting thing so maybe someone can comment where the issue is.

Carnë Draug <carandraug>
Group Member
Sun 10 Sep 2017 03:42:26 PM UTC, comment #5: 

I've had a look at the new version (file #41773), and this looks much better to me. Thanks for the changes.

Let's wait now, what the image package maintainer says.

Hartmut <hardy>
Sun 10 Sep 2017 12:39:22 AM UTC, comment #4: 

Thanks for the feedback, I only noticed that viscircles was missing when testing your imfindcircles.

I addressed all of your comments :)


(file #41771)

Alexander Wilms <alexanderw>
Sat 09 Sep 2017 07:44:34 PM UTC, comment #3: 

Thank you for this nice contribution to the image package, Alexander! (It goes very well with my recently submitted imfindcircles function...)

I've also tested your code (file #41759 from comment #2) with the code examples from the Matlab help page to "viscircles" and to "imfindcircles" and the results look fine.

What follows are some comments to improve your code in order to make the acceptance by the image package maintainer more probable:

  • help text at the beginning of your function:
    • there seems to be a texinfo syntax error in your current code, try "help viscircles" to see it.
    • I think the first explained code syntax should use "deftypefn" where as all the later ones should use "deftypefnx" (note the x as last letter).
    • The "EdgeColor" parameter is undocumented at the moment
    • Could you document all input parameters (and the output parameter) a bit more verbose? The necassary shape of the centers input is of relevance for example.
    • The goal of this is to make the help text useful to prospect users of your function.


  • The general coding style needs some adoption of Octave's coding guidline. See [1] and [2] for details. Some starting points:
    • use snake_case instead of CamelCase for variable names
    • use a space after a funtion call, this is: use "cos (5)" instead of "cos(5)".
    • use "double quotes" instead of 'single quotes' for strings
    • use endif, endfor, endfunction instead of end


  • add some more input checks
    • give a useful error message for "illegal" inputs, e.g. "viscircles ('hello')"
    • you can use the "data type" information of the Matlab help page for this
    • The goal is to give the user a hint what's wrong, if he uses your function in the wrong way. It's not nice to only see "index out of bounds in line 354" ;)


  • add tests to your function (see [2])
    • I'm not sure what are good tests for this plot function
    • But at least you should add some simpel tests to make sure the functions runs without any error on several reasonable input values.
    • The goal of this is to make your code stay useful over the years, when other people start to change it (but they can make sure that your tests still pass).
    • This is most important in my personal opinion.


  • bonus: add a nice "demo" block to your function.


If you are unsure, how to deal with some of those issue, just have a look at the source code of some already existing Octave functions. It's fine to "copy" from there :)

[1] https://sourceforge.net/p/octave/image/ci/default/tree/HACKING
[2] https://wiki.octave.org/Octave_style_guide

Hartmut <hardy>
Thu 07 Sep 2017 06:45:21 PM UTC, comment #2: 

I tested this with the examples given here [0], using imfindcircles.m from [1] and the results look as expected.

[0] https://de.mathworks.com/help/images/ref/imfindcircles.html
[1] https://savannah.gnu.org/patch/?9448

(file #41759)

Alexander Wilms <alexanderw>
Thu 07 Sep 2017 06:38:35 PM UTC, comment #1: 

adding people to cc

Hartmut <hardy>
Thu 07 Sep 2017 08:55:53 AM UTC, original submission:  


Alexander Wilms <alexanderw>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #41777:  viscircles.m added by alexanderw (6KiB - text/plain)
file #41773:  viscircles.m added by alexanderw (6KiB - text/plain)
file #41772:  viscircles.m added by alexanderw (6KiB - text/plain)
file #41771:  viscircles.m added by alexanderw (6KiB - text/plain)
file #41759:  viscircles.m added by alexanderw (3KiB - text/plain)
file #41757:  viscircles.m added by alexanderw (3KiB - text/plain)
file #41756:  viscircles.m added by alexanderw (3KiB - text/plain)
file #41755:  viscircles.m added by alexanderw (3KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2018-01-11 carandraug StatusNone Done
        Open/ClosedOpen Closed
    2017-09-10 alexanderw Attached File- Added viscircles.m, #41777
    2017-09-10 alexanderw Attached File- Added viscircles.m, #41773
    2017-09-10 alexanderw Attached File- Added viscircles.m, #41772
    2017-09-10 alexanderw Attached File- Added viscircles.m, #41771
    2017-09-07 alexanderw Attached File- Added viscircles.m, #41759
    2017-09-07 hardy Carbon-Copy- Added avinoam
        Carbon-Copy- Added carandraug
        Carbon-Copy- Added hardy
    2017-09-07 alexanderw Attached File- Added viscircles.m, #41757
    2017-09-07 alexanderw Attached File- Added viscircles.m, #41756
    2017-09-07 alexanderw Attached File- Added viscircles.m, #41755

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code