Conversation
bdelespierre
force-pushed
the
v3
branch
8 times, most recently
from
38afdbc to
05a1f2b
Compare
| namespace Kmeans; | ||
|
|
||
| use Kmeans\Cluster; | ||
| use Kmeans\ClusterCollection; |
There was a problem hiding this comment.
Import 'Kmeans\Cluster' and 'Kmeans\ClusterCollection' are never used
| namespace Kmeans\Interfaces; | ||
|
|
||
| use Kmeans\Interfaces\ClusterCollectionInterface; | ||
| use Kmeans\Interfaces\PointCollectionInterface; |
There was a problem hiding this comment.
ClusterCollectionInterface and PointCollectionInterface are the same namespace as InitializationSchemeInterface.
Therefore, "use Kmeans\Interfaces\ClusterCollectionInterface" and "use Kmeans\Interfaces\PointCollectionInterface" can be deleted unless they are intended to be used explicitly.
|
|
||
| interface AlgorithmInterface | ||
| { | ||
| public function clusterize(PointCollectionInterface $points, int $nbClusters): ClusterCollectionInterface; |
There was a problem hiding this comment.
In that discussion we decided to add setDistanceFunction to AlgorithmInterface .
There was a problem hiding this comment.
Yes, in another PR 👍
|
Really great! I checked the implementation and proceeded with the review. I just made a few comments. |
src/Algorithm.php
Outdated
| $this->iterationCallbacks[] = $callback; | ||
| } | ||
|
|
||
| public function clusterize(PointCollectionInterface $points, int $nbClusters): ClusterCollectionInterface |
There was a problem hiding this comment.
The return of Algorithm::clusterize is discussed as ClusterizationResultInterface .
There was a problem hiding this comment.
Yes, in another PR +1
|
|
||
| namespace Kmeans\Interfaces; | ||
|
|
||
| interface ClusterizationResultInterface extends \Serializable |
There was a problem hiding this comment.
We decided to add getTotalVariance() .
There was a problem hiding this comment.
Yes, in another PR +1
|
Thanks, @battlecook this PR is an Epic, more smaller PRs are going to go from When we're happy with the result, we can prepare a RC from V3. |
2 participants
No description provided.