New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify KMeans code #186

Closed
wants to merge 18 commits into
base: svn-trunk
from

Conversation

Projects
None yet
3 participants
@Komzpa
Member

Komzpa commented Jan 9, 2018

@codecov

This comment has been minimized.

codecov bot commented Jan 9, 2018

Codecov Report

Merging #186 into svn-trunk will decrease coverage by <.01%.
The diff coverage is 91.01%.

Impacted file tree graph

@@              Coverage Diff              @@
##           svn-trunk     #186      +/-   ##
=============================================
- Coverage      79.24%   79.24%   -0.01%     
=============================================
  Files            202      201       -1     
  Lines          62927    62893      -34     
=============================================
- Hits           49868    49839      -29     
+ Misses         13059    13054       -5
Impacted Files Coverage Δ
liblwgeom/lwkmeans.c 85.34% <91.01%> (+2.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bd5d2e...99aebab. Read the comment docs.

if (num_objs <= 0) return;
weights = lwalloc(sizeof(int) * config->k);

This comment has been minimized.

@pramsey

pramsey Jan 10, 2018

Member

Consider moving this to config, so you don't have to alloc/free it on each iteration.

This comment has been minimized.

@Komzpa

Komzpa Jan 10, 2018

Member

Moved out of function. Got rid of config altogether, -50 lines.

Komzpa added some commits Jan 10, 2018

@Komzpa

This comment has been minimized.

Member

Komzpa commented Jan 10, 2018

@pramsey @tomvantilburg please take a look and wave an OK if it's fine to commit.

@tomvantilburg

This comment has been minimized.

tomvantilburg commented Jan 10, 2018

I'm happy with the new implementation. It is certainly faster, and much better than what we had 👋
Just for the looks of it, here's the result of the clustering on a regular grid (bluer is larger maxdistance, from 1.4 to 4.5):
image

@tomvantilburg

This comment has been minimized.

tomvantilburg commented Jan 10, 2018

And the situation as it was previously (curiously, dropping 2 clusters out of 1000):
image

@pramsey

This comment has been minimized.

Member

pramsey commented Jan 10, 2018

👋

@strk strk closed this in aad0e36 Jan 10, 2018

@Komzpa Komzpa deleted the KMeans-rewrite branch Jan 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment