Samanthacampo/kmeans++ #34
Conversation
Craigacp
left a comment
There was a problem hiding this comment.
Few small changes, mostly documentation.
Clustering/KMeans/src/main/java/org/tribuo/clustering/kmeans/KMeansTrainer.java
Show resolved
Hide resolved
| /** | ||
| * Initialisation method called at the start of each train call. | ||
| * Initialisation method called at the start of each train call when | ||
| * using uniform centroid initialisation. |
There was a problem hiding this comment.
I think this should say "Centroids are initialised using a uniform random sample from the feature domain" somewhere in the javadoc.
|
|
||
| // go through every vector and see if the min distance to the | ||
| // newest centroid is smaller than previous min distance for vec | ||
| double tempDistance; |
There was a problem hiding this comment.
There's no need to define tempDistance outside the for loop. Better to keep it inside so the compiler doesn't think it actually lives any longer.
Clustering/KMeans/src/main/java/org/tribuo/clustering/kmeans/KMeansTrainer.java
Show resolved
Hide resolved
| public int iterations = 10; | ||
| @Option(charName='d',longName="distance",usage="Distance function to use in the e step. Defaults to EUCLIDEAN.") | ||
| public Distance distance = Distance.EUCLIDEAN; | ||
| @Option(charName='i',longName="initialisation",usage="Type of initialisation to use for centroids. Defaults " + |
There was a problem hiding this comment.
There already is a i argument, so this options class is invalid, could you change it to something else?
There was a problem hiding this comment.
Also, there's no need to put the default into the usage string, I updated the options code to print that automatically. While you're in here could you remove the default bit from the other usage strings? I missed that in the docs pass I did.
Add kmeans++ centroid initialization option to kmeans.