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

WIP Atkin sieve #172

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

WIP Atkin sieve #172

wants to merge 18 commits into from

Conversation

Bodigrim
Copy link
Owner

Closes #157

It is already competitive with the existing sieve and is 10 times simpler, but I need to merge more performance patches into https://github.com/Bodigrim/bitvec to make this code really shine.

@Bodigrim
Copy link
Owner Author

This is more or less ready per se. But unfortunately prime counting is so intimately (and impenetrably) connected to the existing Eratosthenes sieve, that I cannot decomission the latter. That said, this PR is on hold until I disentangle this knot.

@rockbmb
Copy link
Contributor

rockbmb commented Mar 12, 2020

Hey @Bodigrim,

unfortunately prime counting is so intimately (and impenetrably) connected to the existing Eratosthenes sieve, that I cannot decomission the latter.

I understand that merging this as is without removing the earlier sieve code would just clutter the codebase (even further), but if it's already working with good performance, wouldn't merging it now and making a new PR that proceeds with decoupling the Math.NumberTheory.Primes.Counting API from Math.NumberTheory.Primes.Sieve.Eratosthenes also work?

@Bodigrim
Copy link
Owner Author

My main goal here is to make source code more maintainable. I can even agree to worsen performance mildly.

It might happen that I'd never untangle Math.NumberTheory.Primes.Counting. Having two implementations of sieves side-by-side is not something future maintainers would be thankful for ;)

@rockbmb
Copy link
Contributor

rockbmb commented Mar 14, 2020

@Bodigrim can't say I don't understand. It's already difficult enough maintaining the package as it is - I remember trying to migrate from array to vector, that was challenging.

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.

Implement the sieve of Atkin
2 participants