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

 
 

patch #9743: [octave forge] (statistics) silhouette function

Submitter:  None
Submitted:  Tue 08 Jan 2019 03:11:58 PM 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 12:18:38 PM UTC, comment #15: 

BTW, I had a look at pdist.m's input validation (see comment #5) but that is too much involved for me for now.

Also I didn't hear back from Nan Zhou.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 27 Mar 2021 11:53:10 AM UTC, comment #14: 

Thanks very much, pushed here:
http://hg.code.sf.net/p/octave/statistics/rev/0188886f27ea

This and the many other contributions you've submitted will be part of the next statistics package release (I suppose 1.4.3).

Closing report.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 25 Mar 2021 07:49:08 PM UTC, comment #13: 

This is my definitive patch. A few tweaks for slightly better performance, corrected a few minor bugs for edge cases.

(file #51140)

Anonymous
Mon 15 Mar 2021 10:51:38 PM UTC, comment #12: 

Correct. I think that the function could be made more efficient, because right now it can kill your pc with large datasets (with a size of tens of thousands of elements), but otherwise it is fine.

Side note: I guess that MATLAB employs an "improved" version of this function. Since a few bad elements can make look bad an entire cluster, MATLAB probably weeds out extreme values when computing the mean distance between a point and all the other points. That way its silhouette plot looks better. It makes sense, since detecting and removing outliers is so common in statistical analysis (for good reasons and sometimes for bad reasons, too).

Anonymous
Mon 15 Mar 2021 03:32:44 PM UTC, comment #11: 

You mean, the bug announced in comment #9 was "invalid" & the patch in comment #8 is fine?

Philip Nienhuis <philipnienhuis>
Group Member
Mon 15 Mar 2021 12:42:19 PM UTC, comment #10: 

Nothing substantial. I just tested this silhouette function against sklearn silhouette function and their results are (almost) the same (up to the fourth digit). It is MATLAB that looks a bit different and I don't know why.

However since this silhouette function renders consistent results, that is they make sense and stick to the definition of silhouette value, I think that this silhouette function works as intended (even though there might be some magnitude inconsistencies with MATLAB).

Anonymous
Sun 14 Mar 2021 10:30:30 PM UTC, comment #9: 

Hold on. I indeed found a little bug, I'll fix it soon..

Anonymous
Sun 14 Mar 2021 09:25:50 PM UTC, comment #8: 

New patch:

pdist (X, metric, varargin)


becomes

pdist (X, metric, varargin{:})


(file #51064)

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

Yeah no one can see who put up comment #0, so I just emailed the author just in case.

As to kmeans:
Other(s) chimed in there so I'll just let the dust settle down.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 13 Mar 2021 04:50:26 PM UTC, comment #6: 

Ah, I'll try to answer to your questions:

  • no, I quite naively thought that comment #1 was by the original author and so I thought he was OK with including his work in Octave.
  • I suspect those differences are the result of better clustering by MATLAB, that is their 'kmeans' function works better (I made a little patch #10042 but that does not improve accuracy).
  • I tested this silhouette function against MATLAB with some fixed dataset (no 'randn', no 'kmeans') and their outputs are identical; unless something strange happens with larger (>100 samples) datasets, which I doubt, there are no reasons to worry.
  • Yes, that is a bug. Just noticed that 'varargin' is a cell so it must be converted before passing it to 'pdist' (i.e. line 119: varargin -> varargin{:}).
Anonymous
Sat 13 Mar 2021 03:30:34 PM UTC, comment #5: 

Thanks.

  • did you ask the original author if he/she agrees with absorbing his/her work into Octave's statistics package? It's GPL 3 so not strictly needed but I find it polite to do so. If you didn't yet, I'll ask.
  • I ran the silhouette demo in Octave and Matlab, see attached pic. There are differences, is there a reason to worry? (again, I have no clue, just asking you as the expert)
  • I ran the examples on the matlab doc page, all examples gave small differences with ML's output. Again, significant?
  • The 4th and last example gave an error in Octave-7.0.0:


>> s1 = silhouette(X,clust,chiSqrDist,w)
error: binary operator '*' not implemented for 'matrix' by 'cell' operations
error: called from
    @<anonymous> at line 1 column 22
    pdist at line 197 column 18
    silhouette at line 119 column 16


Could also be a bug in pdist. So I started digging and the result made me suspect that pdist.m gets fed an improper argument shape (i.e., from silhouette.m).  If so I'd think pdist.m also needs stricter input validation, but I have no time now to dive into it.



Philip Nienhuis <philipnienhuis>
Group Member
Thu 11 Mar 2021 11:38:22 AM UTC, comment #4: 

Here it is.

(file #51045)

Anonymous
Wed 10 Mar 2021 08:52:31 PM UTC, comment #3: 

Patch looks good but lacks

  • addition of silhouette to Index.in
  • an entry in the NEWS file


Philip Nienhuis <philipnienhuis>
Group Member
Wed 10 Mar 2021 07:14:38 PM UTC, comment #2: 

I used Nan Zhou's code to write a patch conformant to Octave guidelines. If I remember correctly it should be feature complete (or almost feature complete).

(file #51037)

Anonymous
Sun 13 Jan 2019 07:48:01 PM UTC, comment #1: 

Thank you for this contribution. Very good Octave coding style.

A quick look shows that it needs some work:

  • It needs tests
  • The nested functions had better be moved outside the main function


Then still some nitpicking (apologies!) as regards Octave coding style:
- endfunction rather than end
- start comment lines with ## rather than %
- texinfo header rather than plain text
- space between function names and their arguments
For examples take about any .m-file function from the statistics package.


Philip Nienhuis <philipnienhuis>
Group Member
Tue 08 Jan 2019 03:11:58 PM UTC, original submission:  

perhaps we can integrate this?
https://github.com/solo7773/silhouette_Octave

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51140:  silhouette.diff added by None (8KiB - text/x-patch - polished version)
file #51064:  silhouette.diff added by None (8KiB - text/x-patch - varargin -> varargin{:})
file #51045:  silhouette.diff added by None (8KiB - text/x-patch - +index.in +NEWS)
file #51037:  silhouette.diff added by None (7KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

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-27 philipnienhuis StatusIn Progress Done
    Open/ClosedOpen Closed
2021-03-25 None Attached File- Added silhouette.diff, #51140
2021-03-14 None Attached File- Added silhouette.diff, #51064
2021-03-13 philipnienhuis StatusNone In Progress
    Assigned toNone philipnienhuis
2021-03-13 philipnienhuis Attached File- Added silhouette_comp-ML-Oct.png, #51055
2021-03-11 None Attached File- Added silhouette.diff, #51045
2021-03-10 None Attached File- Added silhouette.diff, #51037
2019-01-08 mtmiller Summary[octave-forge statistics] silhouette function [octave forge] (statistics) silhouette function

Back to the top

Powered by Savane 3.13-02a9.
Corresponding source code