Skip to content
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

arrayType #244 with tests #249

Closed
wants to merge 3 commits into from
Closed

Conversation

andrewharvey
Copy link
Contributor

I've added tests for #244

@HarelM
Copy link

HarelM commented Aug 27, 2024

@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
Copy link
Contributor Author

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
Copy link
Member

mourner commented Aug 28, 2024

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
Copy link

HarelM commented Aug 28, 2024

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

@andrewharvey
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants