[Octave-patch-tracker] [patch #9930] [octave forge] (statistics) boxplot function with extended features

[Philip Nienhuis]

Follow-up Comment #7, patch #9930 (project octave):

Thanks Andreas.

IMO you did quite a good job of converting to Octave coding style.
Yet I spotted a few more style issues, but I noted much of that was already
(or still) in the original version even before you started editing :-)
(Note: I'm sure I'm not quite the most fanatic style nitpicker here ;-) )
To wit:

* Enclose text strings as much as possible within double quotes, not single
quotes;

* Be careful to match 'if' with "endif" etc;

* Keep line length within 80 chars, use "...' continuation

* As of Octave 7 (dev version), expressions at the RHS with empty {} indices
are no longer allowed, so better use {:}

* Don't add ".m" suffix to function name in warning/error messages

* Try to always include spaces around operators, between function names and
first parenthesis, and after each comma that separates function arguments.

In a few spare hours I've done this now myself, compared the function with
Matlab's and tried a few of their examples, and I also edited the texinfo to
make it a better readable.
IMO it became much more worthwile now, thanks for that.

In addition I:
- replaced all strcmp calls by strcmpi (Matlab proved to be case-insensitive)
- made the whisker widths dependent on the box widths
- made output arguments dependent of explicit user output request
- reset hold status to off at the end of the function (but see below).

Added to this comment is
* new boxplot.m (boxplot_v3.m, rename to boxplot.m before use)
* patch of it relative to your last version (v2->v3) to show you what I did
* complete patch relative to original boxplot.m (orig->v3)

Although I think it is already good enough for replacing the current
octave-forge version, there's still improvement possible, in (subjective)
order of decreasing importance:

* Actually the function needs more input validation. e.g., '"boxstyle", "any"'
leads to filled plots, IMO boxplot should better flag that as an unrecognized
option value.

* Your second demo has irregularly drawn outlines, I think because the fill is
drawn after (& partly over) the outline. Better first draw the fill and then
the outline.

* Maybe first check & save current hold state before plotting proper (use
ishold() ) and restore it at the end of the function.

* The groups argument currently only accepts numerical input AFAICS, but one
of the simple Matlab examples has cellstr input. For better Matlab
compatibility this should be implemented as well.

Will you pick these up?

In addition a real Mercurial cset would be desirable, incl. NEWS item. But I
can do that for you if you never used Mercurial.


(file #49216, file #49217, file #49218)
    _______________________________________________________

Additional Item Attachment:

File name: boxplot_v3.m                   Size:25 KB
    <https://savannah.gnu.org/file/boxplot_v3.m?file_id=49216>

File name: boxplot_v2-v3.patch            Size:34 KB
    <https://savannah.gnu.org/file/boxplot_v2-v3.patch?file_id=49217>

File name: boxplot_orig-v3.patch          Size:31 KB
    <https://savannah.gnu.org/file/boxplot_orig-v3.patch?file_id=49218>



    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/patch/?9930>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/