Issue #64: added n_init to kmeans #78

lbollar · 2016-09-30T20:06:37Z

Sorry, I am still figuring out the pull request process and think I accidentally closed last one. Here is new request with suggested cleanups.

Apologies.

EDIT: fixes #64, original PR #77

ararslan · 2016-09-30T20:40:32Z

src/kmeans.jl

-            maxiter=maxiter,
-            tol=tol,
-            display=display)
+	n_init > 0 || throw(ArgumentError("n_init must be greater than 0"))


Everything is still being indented with tabs. You can do a find-and-replace to change everything at once, if you wish; I do it in Vim like

:%s/\t/ /g

ararslan · 2016-09-30T20:41:10Z

If you accidentally close a pull request, you can just click "reopen," no need to open a new one.

ararslan · 2016-10-05T05:11:56Z

src/kmeans.jl

@@ -72,6 +92,8 @@ function _kmeans!{T<:AbstractFloat}(
    tol::Real,                      # in: tolerance of change at convergence 
    displevel::Int)                 # in: the level of display

+
+


Please remove the excess whitespace here and above. The change is unrelated to the PR.

ararslan · 2016-10-05T05:13:25Z

src/kmeans.jl

+    end
+
+    return bestresult
+


Please remove the line breaks on lines 60, 73, and 75 for style consistency with the rest of the code.

ararslan · 2016-10-05T16:56:52Z

src/kmeans.jl

                tol::Real=_kmeans_default_tol,
                display::Symbol=_kmeans_default_display)
+


Another unrelated whitespace change

ararslan · 2016-10-05T17:06:54Z

Sorry, I realize the whitespace pedantry is probably annoying. Once my outstanding style comments are addressed I think this will be good to go. Thanks for the contribution!

lbollar · 2016-10-05T17:07:48Z

No I completely apologize for these small things that I didn't take care to consider. Thanks for helping me learn about this process. It is preparing me for future commits.

ararslan · 2016-10-05T17:17:45Z

src/kmeans.jl

@@ -71,7 +87,7 @@ function _kmeans!{T<:AbstractFloat}(
    maxiter::Int,                   # in: maximum number of iterations 
    tol::Real,                      # in: tolerance of change at convergence 
    displevel::Int)                 # in: the level of display
-
+    


Trailing whitespace. I don't know what editor you use, but I think Atom trims trailing whitespace by default, and in Vim you can do :%s/\s\+$//g to remove it.

ararslan · 2016-10-05T17:31:52Z

src/kmeans.jl

                tol::Real=_kmeans_default_tol,
                display::Symbol=_kmeans_default_display)

+


One last remaining extraneous newline.

ararslan · 2016-10-05T18:51:43Z

This looks great to me, though I'd like a second pair of eyes on the implementation before it's merged. @KristofferC?

kmsquire · 2017-08-26T14:38:26Z

This is rather old, but Clustering.jl hasn't had any active maintainers in a while. I just fixed the merge conflicts. This seems like a nice addition, but the documentation needs to be updated to reflect the interface change. @lbollar, are you still available to update this?

lbollar · 2017-08-26T16:52:41Z

I'll try and get some time soon and update. On Aug 26, 2017 9:38 AM, "Kevin Squire" <notifications@github.com> wrote: This is rather old, but Clustering.jl hasn't had any active maintainers in a while. I just fixed the merge conflicts. This seems like a nice addition, but the documentation needs to be updated to reflect the interface change. @lbollar <https://github.com/lbollar>, are you still available to update this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#78 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGB4lm6DB9wlG8B7KfKt1mU035r55gbxks5scC3jgaJpZM4KLdq3> .

alyst · 2017-12-06T12:42:54Z

src/kmeans.jl

@@ -44,20 +45,34 @@ function kmeans{T<:AbstractFloat}(X::Matrix{T}, k::Int;
                weights=nothing,
                init=_kmeans_default_init,
                maxiter::Integer=_kmeans_default_maxiter,
+                n_init::Integer=_kmeans_default_n_init,


I understand that n_init comes from Python's sklearn (#64), but it doesn't sound like a best choice for me.
Maybe something like n_tries to reflect that the parameter defines how many times the algorithm, rather than some initialization procedure, is run?

or ntries? And wouldn't be an overkill to run 10 times? I recommend default value 1, because usually a quick partitioning is required and not necessarily best one. And, if one needs to find a best clustering, this parameter can be set to larger value explicitly.

10 is what sklearn does at it sounds reasonable to me.
It isn't unusual to run 1000s of times, (that was done as the baseline for the affinity propagation paper)
If some need a quick partition they can ask for it.

The default shouldn't be so sensitive to random factors.

I think 10 strikes the right balance.
Though I could see argument for 3 or 30

oxinabox · 2018-09-28T11:00:52Z

Any interest in merging this sometime?

alyst · 2018-10-03T18:00:29Z

I think it's worth merging. I'd rename the parameter to ntries. The default (10) seems reasonable given that the method is quite fast.
Obviously, the PR has to be updated to 1.0 (@compat), and the param docs added to kmeans.rst.

lbollar added 2 commits September 30, 2016 13:12

Issue JuliaStats#64: added n_init to kmeans

775eefc

cleaned up for PR

f85cce7

ararslan reviewed Sep 30, 2016

View reviewed changes

removed tabs

3ea7c39

ararslan reviewed Oct 5, 2016

View reviewed changes

removed line breaks to conform with style

fd296d4

ararslan reviewed Oct 5, 2016

View reviewed changes

src/kmeans.jl Outdated

tol::Real=_kmeans_default_tol,

display::Symbol=_kmeans_default_display)

Copy link

Member

ararslan Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another unrelated whitespace change

fixed another whitespace conflict

50f0654

ararslan reviewed Oct 5, 2016

View reviewed changes

used vim command to hopefully fix trailing whitespace issues

a9cb7ab

ararslan reviewed Oct 5, 2016

View reviewed changes

src/kmeans.jl Outdated

tol::Real=_kmeans_default_tol,

display::Symbol=_kmeans_default_display)

Copy link

Member

ararslan Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last remaining extraneous newline.

extraneous newline

b8b381b

ararslan approved these changes Oct 5, 2016

View reviewed changes

Merge branch 'master' into n_init

5ae2901

alyst reviewed Dec 6, 2017

View reviewed changes

alyst added enhancement api labels Feb 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #64: added n_init to kmeans #78

Issue #64: added n_init to kmeans #78

lbollar commented Sep 30, 2016 •

edited by alyst

ararslan Sep 30, 2016

ararslan commented Sep 30, 2016

ararslan Oct 5, 2016

ararslan Oct 5, 2016

ararslan Oct 5, 2016

ararslan commented Oct 5, 2016

lbollar commented Oct 5, 2016

ararslan Oct 5, 2016

ararslan Oct 5, 2016

ararslan commented Oct 5, 2016

kmsquire commented Aug 26, 2017

lbollar commented Aug 26, 2017 via email

alyst Dec 6, 2017

wildart Sep 28, 2018 •

edited

oxinabox Sep 29, 2018

oxinabox commented Sep 28, 2018

alyst commented Oct 3, 2018

		@@ -72,6 +92,8 @@ function _kmeans!{T<:AbstractFloat}(
		tol::Real, # in: tolerance of change at convergence
		displevel::Int) # in: the level of display

		tol::Real=_kmeans_default_tol,
		display::Symbol=_kmeans_default_display)

Issue #64: added n_init to kmeans #78

Are you sure you want to change the base?

Issue #64: added n_init to kmeans #78

Conversation

lbollar commented Sep 30, 2016 • edited by alyst

Choose a reason for hiding this comment

ararslan commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Oct 5, 2016

lbollar commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Oct 5, 2016

kmsquire commented Aug 26, 2017

lbollar commented Aug 26, 2017 via email

Choose a reason for hiding this comment

wildart Sep 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxinabox commented Sep 28, 2018

alyst commented Oct 3, 2018

lbollar commented Sep 30, 2016 •

edited by alyst

wildart Sep 28, 2018 •

edited