search

mapbox / supercluster

A very fast geospatial point clustering library for browsers and Node.
ISC License
2.12k stars 300 forks source link

arrayType #244 with tests #249

Closed andrewharvey closed 3 months ago

andrewharvey commented 3 months ago

I've added tests for #244

HarelM commented 3 months ago

@mourner any chance this can be reviewed and merged? In general, I think that exposing internal implementation of the array type isn't great and this should be handled better internally without API change hopefully, or a parameter saying "high precision" instead. Please let me know how I can help in order to push this forward.

andrewharvey commented 3 months ago

I'm thinking now, it might be best to deal with this internally to supercluster. And importantly is it even worth bothering with Float32Array and just always use Float64Array instead? My benchmark tests showed now difference to memory usage, and the CPU, although the CPU time was 20% more (but I don't know enough about JS internals to know if that's due to this change).

mourner commented 3 months ago

Yeah, I'm not sure I can merge this as is because adding an option for internal array type will only add confusion and complexity while sometimes introducing unintended consequences (such as twice the memory footprint). I don't see how it's possible for it to have no difference when Float64Array takes twice the amount of bytes as Float32Array for the same length.

Ideally the change would be internal, but to come up with the right fix, let's get a clear reproducible test case first.

HarelM commented 3 months ago

@ewelinaBS @andrewharvey can you please provide a jsbin that shows the problem?

andrewharvey commented 3 months ago

I don't see how it's possible for it to have no difference when Float64Array takes twice the amount of bytes as Float32Array for the same length.

Yeah I don't understand, but I was just using the bench.js tool included.

There will be a zoom level if we assume the closest point features still separated by radius * 2 where clustering with a radius would run into the Float32Array precision limit but not a Float64Array precision limit, and we could determine if our cluster maxzoom is greater than that zoom level to swap to Float64Array.

Still I think it would be wise to get to the bottom of the memory situation for Float32Array / Float64Array here to verify that there is indeed a saving to be made.

andrewharvey commented 3 months ago

I'll close this, as it's clear this is most likely not the best solution going forward.

Best to discuss the solution back at #243