Weaken find_first_factor contract to any prime
#325
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It turns out, none of our use cases require us to find the _first_ factor. All we really need is to find _any prime_ factor. (And if the number itself is prime, of course, then the only option will be to return the number itself.) Weakening this contract enables us to take advantage of faster factoring methods that aren't guaranteed to find the _smallest_ factor. Obviously, we could use these faster methods to build a function that satisfies our old contract, by repeatedly applying them to _fully_ factor a number, and then taking the smallest one. But this adds extra computation for no clear benefit. Helps #217.
chiphogg
added
the
release notes: ♻️ lib (refactoring)
geoffviola
approved these changes
Nov 19, 2024
| EXPECT_EQ(find_first_factor(7u * 11u * 13u), 7u); | ||
| EXPECT_EQ(find_first_factor(cube(196961u)), 196961u); | ||
| TEST(FindFactor, FindsFirstFactorWhenFirstFactorIsSmall) { | ||
| EXPECT_EQ(find_prime_factor(7u * 11u * 13u), 7u); |
There was a problem hiding this comment.
Q: Should these tests use AnyOf as well as opposed to equaling the smallest factor?
There was a problem hiding this comment.
Probably, yeah. I was just a little bit lazy when I put the PR up initially. 🙂
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It turns out, none of our use cases require us to find the first
factor. All we really need is to find any prime factor. (And if the
number itself is prime, of course, then the only option will be to
return the number itself.)
Weakening this contract enables us to take advantage of faster factoring
methods that aren't guaranteed to find the smallest factor. Obviously,
we could use these faster methods to build a function that satisfies our
old contract, by repeatedly applying them to fully factor a number,
and then taking the smallest one. But this adds extra computation for
no clear benefit.
Helps #217.