skmeans icon indicating copy to clipboard operation
skmeans copied to clipboard

Possible mutation-related bug

Open sswatson opened this issue 5 years ago • 1 comments

On this line, a data point is assigned to the centroids array by reference:

https://github.com/solzimer/skmeans/blob/17aaa1bcd14aa542910041987b1e8a4285991e77/main.js#L60

This means that as the centroids change later in the algorithm, the underlying data array is also changed. This is undesirable for two reasons: mutating the data array which is passed in could have downstream effects, and even if the data array isn't used subsequently, it makes the algorithm incorrect.

Assuming I'm not missing something here (very possible, as I am not a JS developer), data[idx].slice() would be the fix I would suggest.

sswatson avatar Apr 10 '20 00:04 sswatson

Hi @sswatson,

This issue is related to pull request #12

The algorithm is "working as intended", in the sense of doing the things "as fast as possible". Performing this action will reduce considerably its performance, so I'm looking for a solution that resolve the issue for those that doesn't want its data mutated.

Thanks for the feedback

solzimer avatar Apr 23 '20 12:04 solzimer