patchGNU Octave - Patches: patch #9930, [octave forge] (statistics)...

 
 

patch #9930: [octave forge] (statistics) boxplot function with extended features

Submitter:  Andreas Bertsatos <pr0m1th3as>
Submitted:  Thu 30 Apr 2020 04:10:54 PM UTC
   
 
Category:  Forge : new feature Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  philipnienhuis Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 29 Jun 2020 07:31:58 PM UTC, comment #19: 

@Andreas:
I've pushed my fixes here:
http://hg.code.sf.net/p/octave/statistics/rev/a11d8bb35916

and your latest improvements on top of those ('CapWidths') + some tests here, crediting you:
http://hg.code.sf.net/p/octave/statistics/rev/48aee8892967

Well I think enough has been done now about boxplot.m :-)

Thanks Andreas for your hard work.

The improved boxplot function will be in the next statistics package release, probably 1.4.3.

Closing as "Done".

Philip Nienhuis <philipnienhuis>
Group Member
Sun 28 Jun 2020 05:26:43 PM UTC, comment #18: 

Andreas,
Thanks very much for all the testing.

As to the cap widths, I figured some sort of logarithmic scaling might be handy. Esp. on wide boxplots (say 2 or 3 on a lot) I found the caps ridiculously narrow but on graphs with many boxplots (~20 - 25) they looked just fine.

Later today or tomorrow I'll check your latest version and if there are no issues I'll just push. Additional work on cap widths tuning can be done later on, if required.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 27 Jun 2020 06:54:33 PM UTC, comment #17: 

Back once again :)

Phillip,

I 've used the latest boxplot you supplied through all my research code and it works great without any problem. Even old code that used the positional arguments worked out of the box.

However, I 've made an additional feature update an that's why I ended up reproducing all my figure boxplots once again to test it.

The latest update includes an extra option I added ('CapWidths') which allows the user to define a scaling factor for the cap lengths of the whiskers. I know that you 've changed the size in one of your previous updates and then I switched it back to the original default value for compatibility issues, but during this week's work I made some boxplots for my research results and realized that the the caps were too small. Although I just slightly modified the code to do my work, I realized that this would be a nice and forthcoming extra option for everyone else. So I implemented the extra option in the file I've attached as boxplot_v5.m along with the diff file (boxplot_v4-v5.patch) with respect to your latest version.

The default CapWidths scaling is 1, which leaves the length at the originally default value, i.e. widths / 8 => 0.4 / 8 = 0.05, and the scaling is applied on the widths. Of course, it works accordingly if proportional BoxWidth is selected in user options.

I think it is a useful extra feature and well tested as well :)

Many thanks

(file #49377, file #49378)

Andreas Bertsatos <pr0m1th3as>
Fri 19 Jun 2020 08:46:22 PM UTC, comment #16: 

I'm afraid you got me wrong, sorry, I should have been more clear.
What I did it wait until you got your code improvements right, and only then start fixing issues that were there from before you worked on boxplot. The other way round would have implied much more work for you and thus the risk of a frustrated Octave contributor :-)

The plot if blocks were there from the beginning, before you started working on it. You did copy a lot of duplicate code there but that was due to the way the code was set up. Same for the somewhat involved switch statement setup and several style issues. I can't blame you for those :-)
BTW I'm still pondering about how to morph the vertical and horizontal plotting if blocks into one. Should be possible, the code is essentially the same save swapping X/Y axes.

boxplot can surely use some more testing. If you run into issues please supply the relevant use code snippets, than I (or you if you want) can make BIST tests out of them.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 19 Jun 2020 08:00:47 PM UTC, comment #15: 

Philip,

nice work. I really like the extra "Colors" argument and its implementation that allows grouping as well :)

I can't remember why exactly I added the outermost if statements concerning the plot_opts, but something wasn't working properly in one of my tests and I ended up with that solution.

I will test it extensively during next week with all my code for creating boxplot figures from my data and I will report back.

Many thanks

Andreas Bertsatos <pr0m1th3as>
Fri 19 Jun 2020 01:07:15 PM UTC, comment #14: 

Andreas,
here is what I made of it:

  • boxplot.m
  • patch (diff -u) relative to your latest version
  • cset relative to repo.


what I did:

  1.  Add "Colors" argument - I found just yellow fill a bit dull :-)
  2.  Add two demos to show color options
  3.  Simplify the switch statement - switch / case also accept character or cellstr values
  4.  Made input validation a lot stricter - always first check for proper type/class, only then for acceptable values
  5.  Add about 28 tests for proper operation of input validation
  6.  Add two tests for proper (basic) operation of boxplot
  7.  Improved a few comments
  8.  Made OutlierTags accept numeric 0 or 1 as well
  9.  Polish the texinfo a bit
  10.  Threw out the outermost if blocks in the plot section, as plot() also works if plot_opts is empty
  11.  Made the fill section a separate subfunction (required for Colors argument anyway).


As you can see, input validation is now a large part of the code. That often happens, especially if input args accept both numeric and character values.

I don't claim it's perfect now, not at all; but it does adhere better to Octave code quality standards.

Would you please try the newest boxplot.m and report if there are any issues? I'll wait a week or so before I'l push it to the repo anyway.


(file #49315, file #49316, file #49317)

Philip Nienhuis <philipnienhuis>
Group Member
Wed 17 Jun 2020 08:06:06 PM UTC, comment #13: 

Andreas, for starters I pushed your version with one or two minor edits here, crediting you:
http://hg.code.sf.net/p/octave/statistics/rev/7cbcfb5e4cb8

Many thanks for your contribution so far.

I'll proceed with a more rigorous overhaul. There's a bit of code duplication and program logic could be more efficient and transparent here and there.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 13 Jun 2020 09:27:25 PM UTC, comment #12: 

Thanks very much Andreas, I'll review it within the next days.

Without looking at the code now I think I spotted probably the same issues as you did but I didn't want to spoil your party :-)

What I'll do next:

  • After a cursory review (just check that it works), push it to the repo. That way your contribution is clear.
  • Only then I'll do a more thorough review and push my changes, if any at all.

I don't need mention in the copyright per se, thanks, next generations to come will know I worked on boxplot.m because of the memory of Sourceforge's CVS system :-) That'll do for me.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 13 Jun 2020 06:11:44 PM UTC, comment #11: 

Hi Philip
A few more modifications have been made:

  • fixed a couple of style issues I spotted
  • added extensive validation in the input argument selection
  • grouping now supports cell string input as well as numeric vector
  • if "Labels" are not given by the user, then the unique strings from grouping vector are used instead
  • fixed a bug in validating "Orientation", the code was comparing with 'notched' instead
  • fixed the whisker widths dependent on the box widths to be consistent with original boxplot function (typical length is +- 0.05) so we don't break compatibility with existing code.

(I wouldn't be able to reproduce previous plots for my publications and I suspect this would affect other users as well, so I think it's better if the default value is the same as the current function in statistics package)

  • further modified the whisker widths so they are also proportional to box width. if user opts for fixed BoxWidth then they whisker caps are only proportional to Widths (default is 0.4)


Added to this comment is

    new boxplot.m (boxplot_v4.m, rename to boxplot.m before use)
    a patch relative to your last version (v3->v4)
    a patch relative to original boxplot.m (orig->v3)

I also took the liberty to add your name to the author list (check the email) since you also contributed a lot to this new version.

I didn't have the time to extensively test the new function due to huge workload so please do test before pushing it to the repo. If you don't have the time either, then I should be able to do this next weekend.

I went through all MATLAB's alternative options and IMO there is nothing to add that will be of any benefit. For example, their optional grouping is king of useless, since the same can be achieved by parsing a positional vector like [1,2,4,5] etc. The only other MATLAB capability is the 'compact' type that could be of interest for some people, but it requires significant amount of work for something I never use and I also consider a useless plot for illustrative purposes. Nevertheless, if GNU Octave users request it, then I would be happy to work on that.

Perhaps, you've established by now that I am not a big fun of full MATLAB compatibility, since I couldn't care less about MATLAB. My main interest is that we get useful and bug free functions for the GNU Octave community. They can go along and copy whatever they like and sell it as toolboxes or under they shameless license in MATLAB central. :)

Please, let me know if there is any more changes/corrections/modifications that I should attend on this latest version of boxplot. Thanks a lot.

Andreas




(file #49253, file #49254, file #49255)

Andreas Bertsatos <pr0m1th3as>
Sun 07 Jun 2020 05:17:35 PM UTC, comment #10: 

Just ignore the 2nd bullet in the last series of my suggestions in comment #7, I saw that fill() is always called first already. So the jagged outline is probably due to my monitor.

I see that the functions needs some overhaul anyway; I'll do that after your version has been committed to the repo.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 06 Jun 2020 09:00:29 PM UTC, comment #9: 

Andreas:
Just the coding will do for now, I'll take care of Mercurial.
No hurry either.
BTW I like boxplot; it was fun playing around with it and it definitely is useful.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 06 Jun 2020 05:36:27 PM UTC, comment #8: 

Thanks Philip,

I will continue with the improvements you suggested from where you left (v3) and I will make a test file with a few more complex cases, so you can test in Matlab. I will also go through Matlab's documentation about boxplot again and see whether there is anything else usefull that I may add to boxplot's features.

Hopefully, I should be done by the end of next week if not earlier [work related deadline right now :( ].

I have never used Mercurial before, but I will do some reading and try to figure out about csets. Worst case, I may need some guidance, but at least I will know how to do it for the next time.

Andreas Bertsatos <pr0m1th3as>
Fri 05 Jun 2020 11:15:03 PM UTC, comment #7: 

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)

Philip Nienhuis <philipnienhuis>
Group Member
Mon 01 Jun 2020 08:34:49 PM UTC, comment #6: 

@Andreas:

Test cases are always a big plus.

I happen to have access to Matlab + statistics toolbox so I can try your examples there as well to check for Matlab compatibility. Just try to make sure those examples comply to Matlab syntax.

IMO Matlab sometimes (?often?) adds way too may bells and whistles to their functions (apparently you're agreeing there) which tends to make maintainability somewhat hard.
So, while full-blown compatibility would of course be perfect, smaller steps towards it would be quite preferrable.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 01 Jun 2020 09:42:42 AM UTC, comment #5: 

Hi Phillip,

thanks for reviewing the updated boxplot. It wasn't meant to rush things from my side, it was a mere procrastination to my usual work on a Sunday morning :-)

Let me know if further adjustments/fixes are required or if I can help in anyway, with further examples perhaps. I don't have Matlab to test its respective function, so I only relied on their descriptions on matlab central about the many options they provide in their implementation. Certain things were intentionally left out because IMO they do more harm than good to the end user and my main interest in modifying the existing function was to be able to tag the outliers in the plots so I can use them directly as article figures.

If it would facilitate your testing, I can privately provide certain complex examples from our work in using most of its features but I can't include them as demos because they contain data sets from our work.

Best regards,

Andreas

Andreas Bertsatos <pr0m1th3as>
Sun 31 May 2020 12:32:03 PM UTC, comment #4: 

wow, that's quick Andreas, I wasn't ready for further processing so fast :-)

Currently I'm updating my laptop (removing outdated Win7 and reassigning HD space) and next up same for my desktop PC; a lot of work.
I'll try to find a spell to further review your "boxplot+", will be somewhere next week.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 31 May 2020 07:39:23 AM UTC, comment #3: 

Hi all,

@Philip thank you very much for your feedback. I have made the required changes in code style (at least to the best of my understanding).

Also thanks for pointing out the CRLF / LF difference, somehow my octave editor has been saving all my m files in a windows style despite eos LF being selected in the preferences (or perhaps not in the past). Nevertheless, I have never noticed, so thanks! :)

Please, let me know if any other changes should be made. I have no experience in code collaboration and I can barely use git to update my github repos I use for my science projects. Thus, I am not very familiar with code styles or Mercurial csets. I have attached a patch file with the diff command, which I hope is ok.

@Mike thanks for updating the title of the patch. I realized a bit late about its appropriate format and I couldn't find a way to correct it.

Best regards,

Andreas


(file #49188)

Andreas Bertsatos <pr0m1th3as>
Sat 30 May 2020 04:30:24 PM UTC, comment #2: 

First of all thank you for your contribution, Andreas, much appreciated. It's quite a bit of work, not "just some".

I'm not statistics package maintainer (or maybe I am too, https://octave.sourceforge.io/statistics/index.html lists "The octave community" as maintainer and I feel to be part of it).
Yet I have some remarks & questions, mostly (coding) style.
Please don't be put off, Octave is a volunteer project where the devs have little time and usually don't prioritize spending time on polishing contributions so we rather have contributors polishing their code themselves:

1. We prefer patches ("diff -u <original_file> <modified_file>"), if possible even Mercurial csets, so that (A) we have a quick idea of exactly what changed and (B) the patch or cset can be applied to our local repos directly.

2. Your file uploaded here contained CRLF end-of-lines (Windows style), please set your editor to LF EOLs.
I hit this when trying "diff -u".

3. In the texinfo header we keep sentences two spaces apart, so two spaces after a period.

4. We're used to separate if blocks over separate lines and have the entire if clause in parentheses, so:

if (strcmp (notched, "on"))
 notched = 1;
endif

rather than

if strcmp (notched, "on") notched = 1; endif

Same for for, while and do-while loops, try-catch and unwind_protect blocks and whatnot :-)

5. We begin comments with double #, not %, so

## check for string input: 'fixed' or 'proportional' (default if mis-spelled)

rather than

% chech for string input: 'fixed' or 'proportional' (default if misspelt)

(I'll grant you the spelling mistakes, I'm no native English speaker either :-) )

6. We put each individual statement on a separate line, so:

numarg -= 2;
indopt++;

rather than

numarg -= 2; indopt++;


7. We use "! " for negation, not "~", so

if (! isempty

instead of

if (~isempty

8. You replaced a demo, but IMO you'd better retain it and just add you demo.

If you can upload a patch or cset revised along the above guidelines, I'll test it and try to compare it with Matlab's output.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 07 May 2020 12:02:40 PM UTC, comment #1: 

Hi all,

I have made minor fixes in the boxplot.m function for better handling the separate handles for each element of the plot.

I have also made the function available at Github for everyone interested to use it or test it.

Best regards,

Andreas

(file #49022)

Andreas Bertsatos <pr0m1th3as>
Thu 30 Apr 2020 04:10:54 PM UTC, original submission:  

Hi,

I have implemented some extra functionality (very useful for me at least) to the boxplot.m function available in the statistics package and I would like for anyone with some spare time to test/review and if appropriate to be included in the next 'statistics' package release.

The major new feature is enabling to plot tags next to the points of each outlier. The tags may be plotted as numeric indices of the respective vector data set or as strings containing sample IDs if supplied by the user. This makes it easier to identify them.

The extra functionality includes a lot of MATLAB existing options which have been implemented without braking compatibility with the existing function.

The help file has also been significantly appended to describe all new functionality along with a more elaborate demo.

Any comments would be highly appreciated.

Best regards,

Andreas Bertsatos

Andreas Bertsatos <pr0m1th3as>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #49377:  boxplot_v5.m added by pr0m1th3as (33KiB - text/x-objcsrc)
file #49378:  boxplot_v4-v5.patch added by pr0m1th3as (2KiB - text/x-patch)
file #49315:  boxplot.diff added by philipnienhuis (29KiB - application/octet-stream)
file #49316:  boxplot.cset added by philipnienhuis (30KiB - application/octet-stream)
file #49317:  boxplot.m added by philipnienhuis (33KiB - application/octet-stream)
file #49253:  boxplot_v4.m added by pr0m1th3as (30KiB - text/x-objcsrc)
file #49254:  boxplot_v3-v4.patch added by pr0m1th3as (12KiB - text/x-patch)
file #49255:  boxplot_orig-v4.patch added by pr0m1th3as (36KiB - text/x-patch)
file #49216:  boxplot_v3.m added by philipnienhuis (25KiB - application/octet-stream)
file #49217:  boxplot_v2-v3.patch added by philipnienhuis (34KiB - application/octet-stream)
file #49218:  boxplot_orig-v3.patch added by philipnienhuis (31KiB - application/octet-stream)
file #49188:  boxplot_patch added by pr0m1th3as (25KiB - application/octet-stream)
file #49022:  boxplot.m added by pr0m1th3as (24KiB - text/x-objcsrc)
file #48975:  boxplot.m added by pr0m1th3as (23KiB - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by pr0m1th3as (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 20 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-06-29 philipnienhuis StatusReady For Test Done
        Open/ClosedOpen Closed
    2020-06-27 pr0m1th3as Attached File- Added boxplot_v5.m, #49377
        Attached File- Added boxplot_v4-v5.patch, #49378
    2020-06-19 philipnienhuis Attached File- Added boxplot.diff, #49315
        Attached File- Added boxplot.cset, #49316
        Attached File- Added boxplot.m, #49317
        StatusIn Progress Ready For Test
    2020-06-13 pr0m1th3as Attached File- Added boxplot_v4.m, #49253
        Attached File- Added boxplot_v3-v4.patch, #49254
        Attached File- Added boxplot_orig-v4.patch, #49255
    2020-06-05 philipnienhuis Attached File- Added boxplot_v3.m, #49216
        Attached File- Added boxplot_v2-v3.patch, #49217
        Attached File- Added boxplot_orig-v3.patch, #49218
    2020-05-31 philipnienhuis StatusNone In Progress
        Assigned toNone philipnienhuis
    2020-05-31 pr0m1th3as Attached File- Added boxplot_patch, #49188
    2020-05-29 mtmiller Summaryboxplot function with extended features for the statistics package [octave forge] (statistics) boxplot function with extended features
    2020-05-07 pr0m1th3as Attached File- Added boxplot.m, #49022
    2020-04-30 pr0m1th3as Attached File- Added boxplot.m, #48975

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code