[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Octave-patch-tracker] [patch #9449] image package: new function viscirc

From: Hartmut
Subject: [Octave-patch-tracker] [patch #9449] image package: new function viscircles.m
Date: Sat, 9 Sep 2017 15:44:34 -0400 (EDT)
User-agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

Follow-up Comment #3, patch #9449 (project octave):

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
** The goal of this is to make the help text useful to prospect users of your

* 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
** 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
** 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
** 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


Reply to this item at:


  Message sent via/by Savannah

reply via email to

[Prev in Thread] Current Thread [Next in Thread]