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

Use Baillie-PSW for prime factorization #324

Merged
merged 3 commits into from
Nov 13, 2024
Merged

Conversation

chiphogg
Copy link
Contributor

Formerly, is_prime depended on find_first_factor. Now, we reverse
that dependency! This is good, because while factoring is generally
hard enough that we've built the foundations of global banking on that
difficulty, is_prime can be done in polynomial time --- and now is,
because we're using baillie_psw. We have a static_assert to make
sure it's restricted to 64 bits, but even this could probably be
removed, because there aren't any known counterexamples of any size.

For efficiency reasons, when factoring a number, it's common to do trial
division before moving on to the "fast" primality checkers. We hardcode
a list of the "first N primes", taking N=100 for now. (We could also
compute them at compile time, but this turned out to have a very large
impact on compilation times.) It should be easy to adjust the size of
this list if we decide later: there are tests to make sure that it
contains only primes, has them in order, and doesn't skip any.

The new is_prime is indeed very fast and accurate. It now correctly
handles all of the "problem numbers" mentioned in #217, as well as the
(much much larger) largest 64-bit prime.

One more tiny fix: we had switched to use std::abs in #322, but this
doesn't actually work: std::abs won't be constexpr compatible until
C++23! So as part of this change, we switch back to something that will
work.

Fixes #217.

Formerly, `is_prime` depended on `find_first_factor`.  Now, we reverse
that dependency!  This is good, because while factoring is generally
hard enough that we've built the foundations of global banking on that
difficulty, `is_prime` can be done in polynomial time --- and now is,
because we're using `baillie_psw`.  We have a `static_assert` to make
sure it's restricted to 64 bits, but even this could probably be
removed, because there aren't any known counterexamples of _any_ size.

For efficiency reasons, when factoring a number, it's common to do trial
division before moving on to the "fast" primality checkers.  We hardcode
a list of the "first N primes", taking N=1000 for now.  (We could also
compute them at compile time, but this turned out to have a very large
impact on compilation times.)  It should be easy to adjust the size of
this list if we decide later: there are tests to make sure that it
contains only primes, has them in order, and doesn't skip any.

The new `is_prime` is indeed very fast and accurate.  It now correctly
handles all of the "problem numbers" mentioned in #217, as well as the
(much much larger) largest 64-bit prime.

One more tiny fix: we had switched to use `std::abs` in #322, but this
doesn't actually work: `std::abs` won't be `constexpr` compatible until
C++23!  So as part of this change, we switch back to something that will
work.

Fixes #217.
@chiphogg chiphogg added the release notes: ♻️ lib (refactoring) Under-the-hood changes to library structure label Nov 13, 2024
@chiphogg chiphogg requested a review from geoffviola November 13, 2024 18:50
Express all loop statements in terms of `i`.
Copy link
Contributor

@geoffviola geoffviola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see both real world and theoretical edge cases covered 😄

@@ -60,6 +74,17 @@ TEST(IsPrime, FindsPrimes) {
EXPECT_FALSE(is_prime(196962u));
}

TEST(IsPrime, CanHandleVeryLargePrimes) {
for (const auto &p : {
225'653'407'801ull,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commend: It's good to see some of the values from the issue make it's way into a unit test.

}

// If we're here, we know `n` is composite, so continue with trial division for all odd numbers.
std::uintmax_t factor = first_primes[n_primes - 1u] + 2u;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like we have test coverage for this part. Can we have a set of tests for large composite numbers? We could include some numbers like

9'007'199'254'740'881ull * 2 = 18'014'398'509'481'762

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: added some coverage for small trial primes, large trial primes, and factors beyond trial division.

- Get rid of `ull` again
- Add some coverage for trial division
@chiphogg chiphogg merged commit 7938695 into main Nov 13, 2024
13 checks passed
@chiphogg chiphogg deleted the chiphogg/factoring#217 branch November 13, 2024 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: ♻️ lib (refactoring) Under-the-hood changes to library structure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Large numbers in magnitudes cause compiler crashes or long compilation times
2 participants