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

 
 

patch #9985: [octave forge] (statistics) Add function confusionchart

Submitter:  None
Submitted:  Sat 24 Oct 2020 08:52:10 AM UTC
   
 
Category:  Forge : new function Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  philipnienhuis Originator Email:  -email is unavailable-
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 27 Mar 2021 09:28:15 PM UTC, comment #24: 

@Stefano:
FYI, I noted that during installation of the statistics package a message "unusable help text in ConfusionChartMatrix.m" appeared.
It turns out that the texinfo was incomplete and at the wrong place.
I used inputParser.m (from core Octave) as a template to improve the texinfo & pushed the results here:
http://hg.code.sf.net/p/octave/statistics/rev/6c9db2fee9c6

Something to keep in mind for a next classdef function.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 07 Mar 2021 09:31:43 PM UTC, comment #23: 

Thanks very much, pushed here:
http://hg.code.sf.net/p/octave/statistics/rev/7f1e99ddfa66
with a few minor edits (copyright date, some empty lines added). In a next push I've repaired the copyright dates (after all you first contributed these functions in 2020).

Sorry for the long-winded process.

This will be part of the next statistics package release, hopefully soon as several new functions have been added and improvements made, not in the least by you.

Closing report.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 07 Mar 2021 06:06:11 PM UTC, comment #22: 

Here it is. My main criticism here is that "classdef" is not well documented. Besides that, no problem.

(file #51019)

Anonymous
Sun 07 Mar 2021 02:35:46 PM UTC, comment #21: 

Point taken, yeah I'm sorry that it leads to some extra work for you.

As I wrote elsewhere it's better to cater for dev Octave already now as within a year (I hope) it´ll be released. Having to rush many updates in many OF packages at Octave release time when developers (some of which happen to be package maintainers as well) are usually overloaded anyway is a bad idea.
That said, I do check with current and past releases too, but I care less for past than for future releases.

Dev Octave is stricter than current 6.2.0, but the consequences haven't trickled down through the entire code base yet; well, that's what development branches are for.
I'd rather call it coincidence that your code exposed some of these dev Octave bugs :-)  My apologies that I sometimes misinterpret newly exposed core Octave bugs as deficiencies in contributed code. The explanation is that I know little about classdef and this specific branch of statistics so I can only concentrate on maintainability (style counts there) and user experience (adequate docs and input validation). Checking code correctness for me is mostly limited to checking w. matlab - I have access to some toolboxes.

BTW my patch for the char matrix truncation warning bug has already been accepted and pushed, so no worries.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 07 Mar 2021 10:07:51 AM UTC, comment #20: 

Ok, I'll add the BIST.

I'd like to point out that:

  • there is no warning on Linux with 6.1 and 4.x.
  • the warning refers to the search for the "parent" keyword in the "varargin", so it is alarming, but harmless.
Anonymous
Sat 06 Mar 2021 10:24:07 PM UTC, comment #19: 

If the bug is confirmed all I ask here is BIST tests for input validation and then I'll push it.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 06 Mar 2021 10:20:54 PM UTC, comment #18: 

@comment #16
(crossed with my comment #17)

It's fishy for sure.
A 2x3 char matrix gets drawn nicely as a 2-line text string so the "only the first row is used" subsentence seems rubbish here.
I've posted bug #60187.


Philip Nienhuis <philipnienhuis>
Group Member
Sat 06 Mar 2021 05:35:24 PM UTC, comment #17: 

Inserting
warning ("off", "Octave:charmat-truncated", "local")
before L.762 of ConfusionChartMatrix.m silences it. Well, that should do I suppose.

Other things:

  • Texinfo help should be good enough
  • There are no input BIST checks


I tried the Matlab example but it turned out the Mathworks has split the Deep Learning Toolbox from the Statistics toobox and esp. the example data :-(
But your demos run fine in Matlab 2020b

If you can add BIST tests please (and insert the abovementioned 'warning(...)' line in ConfusionChartmatrix.m0 there's nothing holding me from pushing the lot.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 06 Mar 2021 02:59:12 PM UTC, comment #16: 

That is a bug in my opinion.

>> help text
'text' is a variable

 -- text (X, Y, STRING)
 -- text (X, Y, Z, STRING)
 -- text (..., PROP, VAL, ...)
 -- text (HAX, ...)
 -- H = text (...)
     Create a text object with text STRING at position X, Y, (Z) on the current axes.

     Multiple locations can be specified if X, Y, (Z) are vectors. Multiple strings can be specified with a character matrix or a cell array of strings.


"Multiple strings can be specified with a character matrix or a cell array of strings."

Anonymous
Sat 06 Mar 2021 02:37:25 PM UTC, comment #15: 

Happens because __plt_get_axis_arg__m is fed the following args:

varargin =
{
  [1,1] =
   Columns 1 through 9:
     1.5000   1.5000   2.5000   3.5000   3.5000   4.5000   4.5000   5.5000   5.5000
   Columns 10 and 11:
     6.5000   7.5000
  [1,2] =
   Columns 1 through 9:
     0.5000   2.5000   0.5000   3.5000   5.5000   1.5000   4.5000   3.5000   5.5000
   Columns 10 and 11:
     2.5000   7.5000
  [1,3] =
1
1
1
1
1
1
3
1
1
1
2
  [1,4] = parent
  [1,5] = -71.118
}


where

debug> class (varargin{3})
ans = char


It's a bit of an obscure warning emitted by octave_char_matrix_str() in ov-str-mat.cc, which is invoked by strcmpi(). Maybe the warning can be locally silenced by adding:

warning ("off", "Octave:charmat-truncated", "local")

somewhere in ?confusionchart.m?  or ?ConfusionChartMatrix.m? but it's probably better to find out why/if this char array is needed in the first place and if a char matrix can be somehow circumvented (it seems to be cooked up in method draw() on L.762-763 of ConfusionMatrixChart.m).

Philip Nienhuis <philipnienhuis>
Group Member
Sat 06 Mar 2021 02:36:00 PM UTC, comment #14: 

On 6.x and older it works. It is certainly this part (line 821):

num2str (l_rs(nonzero_idx))


Where "l_rs(nonzero_idx)" is a column of numbers and "num2str (l_rs(nonzero_idx))" is a matrix of characters, e.g.:

>> num2str ([1;21;3])
ans =

 1
21
 3


The "text" function of line 820 should return an array of "text" graphics objects with the x, y, and label properties determined by each line of "x_rs", "y_rs", "l_rs". A "for" statement could easily overcome the problem, but it is nicer this way, I think.

Anonymous
Sat 06 Mar 2021 01:48:13 PM UTC, comment #13: 

With all 4 demos of confusionchart I get these warnings on Windows (Octave-7.0.0 fixed for bug #60143):

confusionchart example 4: "Confusion chart in a uipanel"
 h = uipanel ();
 Yt = {'Positive', 'Positive', 'Positive', 'Negative', 'Negative' };
 Yp = {'Positive', 'Positive', 'Negative', 'Negative', 'Negative' };
 cm = confusionchart ( h, Yt, Yp );

ans = Confusion chart in a uipanel
warning: multi-row character matrix converted to a string, only the first row is used
warning: called from
    __plt_get_axis_arg__ at line 52 column 12
    text at line 93 column 27
    draw at line 762 column 10
    ConfusionMatrixChart at line 104 column 7
    confusionchart at line 152 column 6
    __demo__ at line 6 column 5
    demo at line 154 column 7

warning: multi-row character matrix converted to a string, only the first row is used
warning: called from
    __plt_get_axis_arg__ at line 52 column 12
    text at line 93 column 27
    draw at line 796 column 10
    ConfusionMatrixChart at line 104 column 7
    confusionchart at line 152 column 6
    __demo__ at line 6 column 5
    demo at line 154 column 7

warning: multi-row character matrix converted to a string, only the first row is used
warning: called from
    __plt_get_axis_arg__ at line 52 column 12
    text at line 93 column 27
    draw at line 820 column 10
    ConfusionMatrixChart at line 104 column 7
    confusionchart at line 152 column 6
    __demo__ at line 6 column 5
    demo at line 154 column 7


Just hold on, I'll investigate first.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 02 Mar 2021 11:47:38 AM UTC, comment #12: 

New version of the patch. Features:

  • polished coding style
  • revised texinfo
  • fixed a bug when using named colours (e.g. "b" or "green")
  • added a demo for "sortClasses"


Note that bug #43047 prevents the "sortClasses" help to be displayed.

(file #50968)

Anonymous
Mon 01 Mar 2021 07:06:35 PM UTC, comment #11: 

OK, bug #60143 is confirmed, so just concentrate on style & texinfo, once done the functions can be pushed quickly.

[OT]
About Matlab's Deep learning TB: yeah, I think Matlab was essentially fully "out-developed" already a few years ago, it seems they now just invent & reuse/remangle functions merely for the sake of change (in two meanings).

Philip Nienhuis <philipnienhuis>
Group Member
Sun 28 Feb 2021 08:23:18 PM UTC, comment #10: 

I've entered a bug report: bug #60143

Philip Nienhuis <philipnienhuis>
Group Member
Sun 28 Feb 2021 08:03:14 PM UTC, comment #9: 

The "set.FontName: function called with too many outputs" error could be a Windows bug. On my Linux box the demos work fine with Octave-7.0.0.
I'm investigating a little more, hold on.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 28 Feb 2021 11:30:19 AM UTC, comment #8: 

As to the texinfo:
OTOH don't make too much out of it, no manual, just some more structure would suffice.

On the array of labels: yeah, I know too little about confusionchart's use senario's, sorry for that. Would the examples you give in comment #7 do nice in the texinfo ....?
FYI I maintain other OF packages (io and mapping). I'm just helping here to get your contributions in the statistics package because (1) it has no formal maintainer and (2) it could be a long wait until some other developer familiar with it's use cases turns up, and as long as it isn't in the package no one will know about it ==> a chicken and egg problem.
It does mean that rigorous exposure and testing will only happen after a new package release when others start using it and maybe hit issues and report those on the trackers and mailing lists.

What helps here is that you already comply very well to Octave's coding and style demands (whether they are reasonable or not; but they just do exist) so that makes my job easier. Thanks for that.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 28 Feb 2021 09:55:27 AM UTC, comment #7: 

About that array of labels, it can be literally anything.

An array of numbers:

confusionchart(m, [1000 575 12]) # labels are "1000", "575", "12"
confusionchart(m, [3.14 1.41 3]) # labels are "3.14", "1.41", "3"


An array of characters:

confusionchart(m, ['a' 'b' 'c']) # labels are "a", "b", "c"
confusionchart(m, ["abc"]) # idem
confusionchart(m, ["abc"]') # idem



An array of strings:

confusionchart(m, ["abc"; "cbc"; "ddd"]) # labels are "abc", "cbc", "ddd"
confusionchart(m, ["abc", "cbc", "ddd"]) # this obviously does not work in Octave, but it should when string arrays will work


A cell string array:

confusionchart(m, {"a", "b", "c"})
confusionchart(m, {"a"; "b"; "c"})


Anonymous
Sat 27 Feb 2021 11:02:02 PM UTC, comment #6: 

OK, I'll try to fix the texinfo and the other things. Even though I have no clue about that Octave 7 error.

I use Octave 6.1 compiled with AMD libraries for BLAS/LAPACK (libblis and libflame) and all those demonstrations are instantaneous on my desktop pc. You should try to run them in a different order.

Deep learning sounds like a marketing scam to sell old statistics as new...

Anonymous
Sat 27 Feb 2021 10:12:08 PM UTC, comment #5: 

FTR I also checked in Octave-6.2.0 (just released) and there it all works nicely.

BTW, I was a bit surprised that the 2nd-3rd-4th demos took a lot longer to get computed than the 1st, while the 1st seems more complicated. But admittedly I know little about this part of statistics (I used to be much more acquainted with geostatistics and TSA), so it's probably just my ignorance at play here :-)

Philip Nienhuis <philipnienhuis>
Group Member
Sat 27 Feb 2021 09:48:14 PM UTC, comment #4: 

Thanks.

I tried 'demo confusionchart' but got this error:

>> demo confusionchart
confusionchart example 1: 1
 Yt = [8 5 6 8 5 3 1 6 4 2 5 3 1 4]';
 Yp = [8 5 6 8 5 2 3 4 4 5 5 7 2 6]';
 confusionchart (Yt, Yp, "Title", ...
   "Demonstration with summaries","Normalization",...
   "absolute","ColumnSummary", "column-normalized","RowSummary",...
   "row-normalized")

ans = 1
confusionchart example 1: failed
set.FontName: function called with too many outputs


This is a new error check introduced in (dev) Octave-7.0.0 (also for inputs). Although 7+ is not yet current I think it's better to fix it already now so we don't have to do that when Octave-7.1.0 is released.
Could you please have a look at this? I tried myself, I do know classdef but I'm not very proficient in the classdef nitty-gritty when fixing bugs.
If you're on Windows and need Octave-7.0.0 I can upload a dev version for you. On Linux you'd need to build Octave-7.0.0 yourself I'm afraid.

Style issues:
- put all if clauses between parentheses (e.g., L.296 in ConfusionMatricxChart.m and L.55 in confusionchart)
- make sure all strings are enclosed in double not single quotes (e.g., L.60 in confusionchart.m)

I've fixed these style things in optimalleaforder.m / patch #10034 (to be pushed soon), but for these 2 funcs I'll leave it up to you.

IMO also the texinfo help texts should be just a little more elaborate, that is, a structured listing of all inputs and outputs, what they are, esp. what they should look like (e.g., along the lines of "Nx4 matrix of doubles", which column or row contains what data, required orientation/dimension of vectors, allowed variations), can they be empty, can they be omitted, what are default values, maybe which function(s) can produce them; "array of labels" => "cellstr array of labels" (Nx1 or 1xN, or doesn't that matter?). That sort of detail. All meant to aid users when things don't go as they hoped for.
As the texinfo currently stands it implicitly assumes quite a bit of knowledge that you and I as experienced Octave users take for granted but less experienced users simply don't have.

Hint: use @itemize / @item / @end itemize macros to enumerate multiple inputs in the order that the function expects them. In some Octave functions @table @asis / @end table is used for this. Similar for outputs. I think I've given a Texinfo style URL in an earlier patch report.

BTW I saw Matlab has confusionchart both in the Statistics and machine learning toolbox (that I have access to) but also in the Deep learning toolbox - never heard of that until today :-)

Philip Nienhuis <philipnienhuis>
Group Member
Sat 27 Feb 2021 05:25:48 PM UTC, comment #3: 

Here it is the new patch: it uses patch #10034 "add function optimalleaforder".

(file #50938)

Anonymous
Sun 31 Jan 2021 04:49:22 PM UTC, comment #2: 

I am currently trying to complete my implementation of 'optimalleaforder'. Once I am done with that I will surely fix this one, too, including the 'sortClasses' method. I think it will be done soon; do not worry, just wait a little bit.

Anonymous
Mon 25 Jan 2021 08:09:04 PM UTC, comment #1: 

Now that linkage, inconsistent and cluster are in the statistics package, would you be able to work on the sortClasses method?

Otherwise I'll do a cursory review, if OK push, then fix any style issues I fell need fixing (can't be many, I think you're doing a Very Good Job). Goodies like sortClasses() can be added later on then.

My pref would be to do it all at once, but as author this is your pick :-)

Philip Nienhuis <philipnienhuis>
Group Member
Sat 24 Oct 2020 08:52:10 AM UTC, original submission:  

This patch adds the confusionchart function and the ConfusionMatrixChart class (using classdef).

The confusionchart function creates a ConfusionMatrixChart object, which is a self-contained plot, i.e. it has its own axes.

It can be optionally drawn inside an existing figure or an existing uipanel, while in MATLAB its parent can be a figure, a panel, a tab, a grid layout etc.

Since it is the object of a class, its properties can be edited after its creation through the dotted notation, e.g. :

cmcobject.Title = "Title of the confusion chart"

Properties implemented: XLabel, YLabel, Title, FontName, FontSize  , DiagonalColor, OffDiagonalColor, Normalization, ColumnSummary, RowSummary, GridVisible.

Properties not implemented: FontColor, PositionConstraint, OuterPosition, InnerPosition, Position, Units, Layout, HandleVisibility.

To sort the classes according to some specific order, one can use the sortClasses method, e.g.:

sortClasses( cmcobject, "auto" )

The sortClasses method currently cannot sort the classes according to the "cluster" ordering, because that needs some work with the linkage and cluster functions.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51019:  confusionchart.diff added by None (36KiB - text/x-patch - tests added)
file #50968:  confusionchart.diff added by None (35KiB - text/x-patch - new version, see comment #12)
file #50938:  confusionchart.diff added by None (33KiB - text/x-patch - conformant to style guidelines; sortClasses with optimalleaforder)
file #50082:  ConfusionMatrixChart.diff added by None (34KiB - text/x-patch - version 1.0)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2021-03-07 philipnienhuis StatusIn Progress Done
        Open/ClosedOpen Closed
    2021-03-07 None Attached File- Added confusionchart.diff, #51019
    2021-03-02 None Attached File- Added confusionchart.diff, #50968
    2021-02-27 philipnienhuis StatusNeed Info In Progress
        Carbon-Copy- Added philipnienhuis
    2021-02-27 None Attached File- Added confusionchart.diff, #50938
    2021-01-25 philipnienhuis StatusNone Need Info
        Assigned toNone philipnienhuis
    2020-10-24 None Attached File- Added ConfusionMatrixChart.diff, #50082

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code