-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
factor: Refactor the factoring logic #1558
factor: Refactor the factoring logic #1558
Conversation
@Arcterus (or any other maintainer) : this fails to build because of the |
That’s in the |
Yes, don't stress out about it :) |
I think this is ready for review as-is. I did not (yet) move the factoring functionality to its own crate, as I realised it's not obvious where to put it, how to integrate with the release process for |
Found a couple minor improvements, should now be clean on Clippy. I'm not doing the last change I mentioned (making |
I’ll try to review this later today. |
No functional change, but prepares a coming optimisation.
Instead, the same `Factors` object is passed around through the execution. ~10% faster.
Useful for printing out in-progress factorisations when debugging.
Non-prime numbers, such as 0 or 1, shouldn't be inserted in the factorisation.
Montgomery only works for odd n, so attempting to construct an instance for an even number results in a panic! The most obvious solution is to special-case even numbers.
I just rebased on Regarding performance, the improvement compared to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Approving to avoid having a PR staying for too long and because it has tests & great coverage.
uu_factor::factor
) from the CLI code and I/O.Improves maintainability and readability.
Refactor things so factoring algorithms return a factor (orNone
) tofactor
.This refactoring will enable implementing multiple factoring methods (and dynamically selecting the best one based on smoothness etc.)
On its own, the performance improvement compared to
master
is pretty negligible, but when combined with #1554 we get another ~15% improvement.