Sat 28 May 2016 10:13:08 AM UTC, comment #28:
Thanks, Nir. I'll close this report, then.
|
Tue 24 May 2016 03:46:11 PM UTC, comment #27:
The revised patch works for me, so I committed it.
|
Sun 22 May 2016 05:46:30 AM UTC, comment #26:
Actually, rather than just reporting i, could you post the whole workspace? Thanks.
|
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)
|
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
|
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)
|
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?
|
Fri 06 May 2016 02:07:58 PM UTC, comment #21:
The user -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)
|
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.
|
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)
|
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
while using
to verify "Start".
|
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).
|
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.
|
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.
|
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.
|
Sun 20 Mar 2016 08:32:13 AM UTC, comment #13:
Thanks, Markus. That certainly saves a lot of code! However, when I try
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.
|
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)
|
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).
|
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)
|
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.
|
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.
|
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)
|
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
|
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?
|
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)
|
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.
|
Fri 09 Jan 2015 12:39:37 PM UTC, comment #2:
Adding maintainer of statistics package to CC list
|
Fri 09 Jan 2015 06:14:57 AM UTC, comment #1:
kmeans is an Octave Forge package. Changing Category.
|
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.
|