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

Fix incorrect results for large numbers of points #134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lkeegan
Copy link

@lkeegan lkeegan commented Jan 16, 2025

- replace all 32-bit integers with 64-bit integers
- previously the results would silently be incorrect if `num_points * k > 2^32-1`
  - caused by integer overflow (see storpipfugl#38)
  - for e.g. k=20 this occurred for ~200 million points (~a few GB of RAM)
- now the results are correct for any (practical) number of points
  - overflow would now only occur for k=20 with ~10^17 points (> trillions of GB of RAM)
  - resolves storpipfugl#38
@mraspaud
Copy link
Collaborator

Thank you for you contribution!

I’m wondering what the effect on performance (speed, memory usage) can be when 32 bits would be enough?

@djhoese
Copy link
Collaborator

djhoese commented Jan 16, 2025

I am also concerned about performance. I think the "safer" solution, although much more difficult to implement, would be to use something like cython fused types and then also generate the C code (from the .mako template) with both 32-bit and 64-bit types. I could be wrong, but I think overloading should work fine if two functions in the C code have the same name but accept different input types.

@lkeegan
Copy link
Author

lkeegan commented Jan 16, 2025

I did some local benchmarks and this PR was ~5% slower than the master branch, and used ~50% more RAM.
Are there any standard benchmarks I should try running?

I agree the better solution would be to have both 32-bit int and 64-bit int implementations, and to choose the appropriate one at runtime based on the number of points and value of k provided by the user, although I also agree it would be more difficult to implement :-)

@djhoese
Copy link
Collaborator

djhoese commented Jan 16, 2025

This user just did some updated benchmarks it seems: #121 (comment)

@lkeegan
Copy link
Author

lkeegan commented Jan 17, 2025

See #135 for an alternative with both 32 and 64 bit implementations which avoids making existing performance / ram use worse.

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.

pydktree breaks down for very large number of data points
3 participants