bugGNU Octave - Bugs: bug #43959, statistics package: kmeans...

 
 

bug #43959: statistics package: kmeans incompatibility with Matlab

Submitter:  None
Submitted:  Fri 09 Jan 2015 06:00:10 AM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  jordigh
Originator Name:  Lachlan Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 3.6.4
Operating System:  * GNU/Linux Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 28 May 2016 10:13:08 AM UTC, comment #28: 

Thanks, Nir.  I'll close this report, then.

Lachlan Andrew <lachlan>
Tue 24 May 2016 03:46:11 PM UTC, comment #27: 

The revised patch works for me, so I committed it.

Nir Krakauer <nir_krakauer>
Sun 22 May 2016 05:46:30 AM UTC, comment #26: 

Actually, rather than just reporting i, could you post the whole workspace?  Thanks.

Lachlan Andrew <lachlan>
Sun 22 May 2016 05:43:51 AM UTC, comment #25: 

Odd.  I get
   PASSES 31 out of 31 tests

Could you run it with a breakpoint at line 358 and tell me what the value of  i  is?

I'm also attaching a patch that gets rid of that spurious "d = , normal" output.

(file #37223)

Lachlan Andrew <lachlan>
Sat 21 May 2016 01:51:17 PM UTC, comment #24: 

Thanks. The patch (file #37220) can now be applied, but the last test gives me a error -- can you check on it?

> test kmeans

d = , normal
  *** test
 [cls, c] = kmeans ([1 0; 2 0], 2, "start", [8,0;0,8], "emptyaction", "drop");
 assert (cls, [1; 1])
 assert (c, [1.5, 0; NA, NA])
!!!!! test failed
ASSERT errors for:  assert (c,[1.5, 0; NA, NA])

  Location  |  Observed  |  Expected  |  Reason
   (1,1)          NA          1.5        'NA' mismatch
   (2,1)         1.5           NA        'NA' mismatch
   (1,2)          NA           0         'NA' mismatch
   (2,2)          0            NA        'NA' mismatch

Nir Krakauer <nir_krakauer>
Sat 21 May 2016 04:39:16 AM UTC, comment #23: 

Thanks for looking at this, Nir.

I've attached a Mercurial changeset (the previous diff had to be applied from within the inst directory, with "patch -p0").

For easier reading, I've also attached the whole file, kmeans_doc_and_tests.m

Please let me know if any further changes are required -- this was my first contribution to Octave and had a very rocky start.



(file #37220, file #37221)

Lachlan Andrew <lachlan>
Thu 19 May 2016 03:24:43 PM UTC, comment #22: 

I tried to apply the most recent patch (file #36744) to the Statistics repository, but it failed with "abort: unable to strip away 1 of 1 dirs from kmeans.m". Andrew, can you re-generate it?

Nir Krakauer <nir_krakauer>
Fri 06 May 2016 02:07:58 PM UTC, comment #21: 

The user -email is unavailable- has been having trouble creating an account at savannah and requested on the mailing list to upload his modified version of kmeans.  I am uploading the files on his behalf.

See http://lists.gnu.org/archive/html/octave-maintainers/2016-05/msg00111.html

(file #37089, file #37090)

Carnë Draug <carandraug>
Group Member
Wed 04 May 2016 11:44:59 AM UTC, comment #20: 

I have the exact 3 problems mentioned in
https://savannah.gnu.org/bugs/?func=detailitem&item_id=43959#comment0

Apparently, these problems had been fixed a year ago but are still not merged yet, so i want to make sure that this bug is not forgotten.

Mahene <mahene>
Thu 24 Mar 2016 12:26:21 PM UTC, comment #19: 

It is more complex than that.  Replicates is an integer, not a matrix.  The third dimension of Start must match Replicates if both are given, but if Replicates is not given, then it can be inferred from the size of Start.

Also, K itself is allowed to be empty if Start is specified.

The attached patch has some documentation and self-tests that explain some of the complications.

(file #36744)

Lachlan Andrew <lachlan>
Mon 21 Mar 2016 09:31:51 AM UTC, comment #18: 

The only way I currently can think of is to check this after p.parse () like that



if isnumeric (p.Results.Start)
  if ~(all (size (p.Results.Start) == size (p.Results.Replicates)))
    error("Size of Start and Replicates must be equal")
  endif
endif


while using


char_numeric         = @(x) (@ischar(x) || @isnumeric(x))


to verify "Start".

Markus Bergholz <markuman>
Mon 21 Mar 2016 09:02:08 AM UTC, comment #17: 

Thanks, Markus.  The challenge with "Start" is that we need to verify that the size is consistent with "replicates".  Can we force the order in which inputParser processes the inputs?  Also, unless we use a global variable, I don't see how we can write a function to pass in to inputParser that relies on one of the parameter values (i.e., an output of inputParser).

Lachlan Andrew <lachlan>
Mon 21 Mar 2016 07:55:56 AM UTC, comment #16: 

Take a look at line 35-38. You can define your own functions (scalar_numeric) to verify input variables. So you have to do it with line 43, where "Start" is checked against @char. I bet you can do it too ;) And try to keep it as a mercurial patch.

Markus Bergholz <markuman>
Sun 20 Mar 2016 11:13:42 PM UTC, comment #15: 

Yes, 'start' is compared against char, but if that fails (the 'otherwise' clause at line ~115) then it is interpreted as a matrix.

The existing patch has a lot of code, but perhaps that is unavoidable given the range of options that Matlab supports.  I have no idea how flexible inputParser is.  Let me know if there is anything I can do to help.

Also, I believe that Matlab treats these options as case-insensitive, so it is probably better to use p.CaseSensitive=false.

Lachlan Andrew <lachlan>
Sun 20 Mar 2016 09:29:11 AM UTC, comment #14: 

It is using p.CaseSensitive = true; So it must be

kmeans (data, 4, "Start", init).

But this will fail too. Because in the older version line 108+ `start` was evaluated against a char ( switch case {"sample", "plus", "cluster"} ...). So my inputparser version checks "start" against a char in line 43. e.g. kmeans (data, 4, "Start", "sample") will work.

Markus Bergholz <markuman>
Sun 20 Mar 2016 08:32:13 AM UTC, comment #13: 

Thanks, Markus.  That certainly saves a lot of code!  However, when I try


data = rand (100, 5);
init = rand (4,5);
kmeans (data, 4, "start", init)


I get the error

kmeans : found unmatched parameters at end of arguments list

Also, from looking at the code, I can't see how the third dimension of the argument of "start" is validated against the argument of "iter", and I don't know inputParser well enough to see if that will be easy.

Lachlan Andrew <lachlan>
Fri 04 Mar 2016 09:10:42 AM UTC, comment #12: 

I've made an hg patch from the last patch (hopefully right). Furthermore I've replaced a lot of code while using inputParser instead of parseparams. So a new review would be cool.

(file #36543)

Markus Bergholz <markuman>
Thu 03 Mar 2016 08:06:54 PM UTC, comment #11: 

I guess using inputParser instead of parseparams can avoid some lines of code and keep it more clean. Especially the while switch case lines ... and blocks as in line 99-105 can completely dropped.

Furthermore, don't use i as your iterator variable. i, j, e, ... are already assigned in octave (but I dunno if this is a contributing guideline or not).

Markus Bergholz <markuman>
Thu 02 Jul 2015 01:15:26 AM UTC, comment #10: 

Jordi, I apologise for the poor quality of my previous patches.  This time, I think that the patch is actually ready!  Please take a look at the latest patch.

It implements most of the remaining functionality of Matlab's kmeans that was missing from the earlier patches.  In particular, it allows:
- all of Matlab's initializations, including kmeans++ (Matlab's default, not implemented in Octave's existing kmeans)
- all of the distance measures

as well as the other options provided by the other patches:
- replicates
- MaxIter
- emptyAction.

It also fixes problems with the way it handles unimplemented options (display, onlinephase, options), and gives an error for options not supported by either Octave or Matlab.

It would be great to have this wrapped up, and sorry again for the problems with my earlier patches.



(file #34357)

Lachlan Andrew <lachlan>
Thu 29 Jan 2015 09:42:20 PM UTC, comment #9: 

For try-catch blocks, the informative block closing is end_try_catch, because I suppose endtrycatch would be too difficult to read.

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Thu 29 Jan 2015 09:39:57 PM UTC, comment #8: 

I actually was cleaning up your patches, and stopped doing all of the work somewhere along the way.

I noticed a bug: you didn't close a block properly with endif when checking for keyword arguments. I urge you to use informative block closing! This kind of bug is exactly what informative block closing is suposed to prevent.

I'll look at your new batch of patches later.

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Thu 29 Jan 2015 06:25:33 AM UTC, comment #7: 

Greetings Jordi,

I've noticed that the three patches I submitted earlier don't play nicely together.  Sorry for being so careless...

Here is a patch that fixes those three issues, and a whole lot of other issues with  kmeans.m

1. Implement "distance", "maxiter" options
2. Implement "erroraction=drop".
3. Fix bug in "erroraction=singleton" (It used to re-assign the last sample, rather than the sample furthest from its centroid.)
4. Implement explicit initialization for "start"; give warning for unimplemented arguments for "start", and drop back to "sample".
5. Give errors when invalid options are supplied, and warnings when valid but unimplemented options are supplied.
6. Don't crash if an odd number of arguments is supplied.

I tried to follow all of the coding standards you mentioned.  I tried to use explicit forms of "end", but it seems "endtry" is not valid.  Is that intentional?

Since this changes almost every line of code, I took the liberty of listing myself at the top in the list of authors.  However, I'm happy for you to remove that.

If you are happy with this, I'll create the commit log entry.

Thanks,
Lachlan

(file #32934)

Lachlan Andrew <lachlan>
Mon 19 Jan 2015 03:03:58 AM UTC, comment #6: 

Thanks for fixing the last bits.  I'll note them for next time.

My surname is Andrew.  I wasn't trying to be anonymous, but had struck a snag generating the my Savanah account.  Thanks for your help,
Lachlan Andrew

Lachlan Andrew <lachlan>
Thu 15 Jan 2015 06:17:56 PM UTC, comment #5: 

Thanks. This looks better. There's still a couple of minor issue with following Octave style: we use informative end statements, e.g. endif and endfor instead of plain "end", and we put () around if and while conditions, like in C. I can fix those myself.

There remains the issue of acknowledging your patch in our commit log. We typically do something like "Name Familyname <user@example.com>", so you would be "Lachlan Something <lachlanbis@gmail.com>". Do you want to remain pseudoanonymous or would you like your full name to appear in our commit logs?

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Thu 15 Jan 2015 02:53:03 AM UTC, comment #4: 

Sorry for the kmeans_replicate  patch -- I mistyped the diff command, and didn't check :(

I think everything is fixed now.  As well as the three fixed patches, I'll append a patch for  replicate  that ignores the extra indentation, so you can see the changes more easily.



(file #32828, file #32829, file #32830, file #32831)

Anonymous
Fri 09 Jan 2015 02:51:35 PM UTC, comment #3: 

Why is kmeans_replicates a whole new file?

Please follow Octave style:

  • I see stray tabs. No tabs, please Use spaces for indentation.
  • Spaces for "function_calls ()" but not for "indexing()".
  • Use "!" for negation, not "~".
  • Call "rows ()" or "columns ()", not "size (, 1)" or "size (, 2)".
  • Use NA instead of NaN for missing values.


For bonus points, create a Mercurial cset and follow our commit message guidelines:

    http://wiki.octave.org/Commit_message_guidelines

As to the actual code itself, for the NaN patch...

  • There is no need for the find() call when defining has_no_NaN. Remove it.
  • I would rename "has_no_NaN", which sounds like a scalar, to "data_idx", to emphasise that this is a logical index that only includes where the data is actually complete.
  • Move the definition of final_classes to the very end, since that is where it's used.


The sums patch seems fine, modulo Octave style. I am too lazy to try to untangle the replicates patch, please fix that first.

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Fri 09 Jan 2015 12:39:37 PM UTC, comment #2: 

Adding maintainer of statistics package to CC list

Carnë Draug <carandraug>
Group Member
Fri 09 Jan 2015 06:14:57 AM UTC, comment #1: 

kmeans is an Octave Forge package.  Changing Category.

Rik <rik5>
Group administrator
Fri 09 Jan 2015 06:00:10 AM UTC, original submission:  

The kmeans function is not compatible with Matlab's.  The attached three patches address three issues.

1. Matlab's kmeans ignores samples with NaN values.  Octave's did not.

2. Matlab's kmeans returns a third value that is a vector of the sums of distances between vectors and their classes' examplars.  Octave's returned a single value that is the sum of all of these sum.

3. Matlab's kmeans accepts an option "replicates", which returns the best of multiple runs.  Octave's did not.

The three patches address these problems. The patch  kmeans_sumd.m  should be applied before  kmeans_replicates.patch.

There are several other incompatibilities that will be easy to fix.  I'm happy to fix the easy ones if/when these patches are accepted.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #37220:  kmeans_doc_and_test.m added by lachlan (17KiB - d2l/unknowntype)
file #37089:  kmeans_test.m added by carandraug (673B - text/x-matlab - modified versions by -email is unavailable-)
file #37090:  kmeans_to_send_to_octave.m added by carandraug (7KiB - text/x-matlab - modified versions by -email is unavailable-)
file #36744:  bug_43959_kmeans.diff added by lachlan (19KiB - text/x-diff)
file #36543:  kmeans_hg.patch added by markuman (18KiB - text/x-diff)
file #34357:  kmeans_patch.m added by lachlan (12KiB - text/x-objcsrc)
file #32934:  kmeans.allpatch added by lachlan (10KiB - application/octet-stream)
file #32832:  commit_replicates.txt added by None (121B - text/plain)
file #32833:  commit_NaN.txt added by None (169B - text/plain)
file #32834:  commit_sumd.txt added by None (251B - text/plain)
file #32828:  kmeans_sumd.patch2 added by None (804B - application/octet-stream)
file #32829:  kmeansNaN.patch2 added by None (1009B - application/octet-stream)
file #32830:  kmeans_replicates.patch2 added by None (4KiB - application/octet-stream)
file #32831:  kmeans_replicates.patch_ingoring_indent added by None (2KiB - application/octet-stream)
file #32793:  kmeansNaN.patch added by None (704B - text/x-patch)
file #32794:  kmeans_sumd.patch added by None (808B - text/x-patch)
file #32795:  kmeans_replicates.patch added by None (5KiB - text/x-patch)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by nir_krakauer (Posted a comment)
  • -email is unavailable- added by carandraug (submitted it on the mailing list)
  • -email is unavailable- added by mahene (Posted a comment)
  • -email is unavailable- added by markuman (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by carandraug (Arno Onken - maintainer of the statistics package)
  • -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 group members can vote.

     

    Follow 25 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-05-28 lachlan StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2016-05-22 lachlan Attached File- Added bug_43959_with_commit_msg.v2.cset, #37223
    2016-05-21 lachlan Attached File- Added kmeans_doc_and_test.m, #37220
        Attached File- Added bug_43959_with_commit_msg.cset, #37221
    2016-05-07 jordigh Dependencies- bugs #47871 is dependent
    2016-05-06 carandraug Attached File- Added kmeans_test.m, #37089
        Attached File- Added kmeans_to_send_to_octave.m, #37090
        Carbon-Copy- Added -email is unavailable-
    2016-03-24 lachlan Attached File- Added bug_43959_kmeans.diff, #36744
    2016-03-04 markuman Attached File- Added kmeans_hg.patch, #36543
    2015-07-02 lachlan Attached File- Added kmeans_patch.m, #34357
    2015-01-29 lachlan Attached File- Added kmeans.allpatch, #32934
    2015-01-15 jordigh StatusNone Patch Submitted
        Assigned toNone jordigh
    2015-01-15 None Attached File- Added commit_replicates.txt, #32832
        Attached File- Added commit_NaN.txt, #32833
        Attached File- Added commit_sumd.txt, #32834
    2015-01-15 None Attached File- Added kmeans_sumd.patch2, #32828
        Attached File- Added kmeansNaN.patch2, #32829
        Attached File- Added kmeans_replicates.patch2, #32830
        Attached File- Added kmeans_replicates.patch_ingoring_indent, #32831
    2015-01-09 carandraug Summarykmeans incompatibility with Matlab statistics package: kmeans incompatibility with Matlab
        Carbon-Copy- Added -email is unavailable-
    2015-01-09 rik5 Carbon-CopyRemoved 72865 -

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code