-
-
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: Add 32b variant for modular arithmetic #1554
Conversation
I'm not sure what's the way forward there: ideally, I can make |
I successfully generalised the function for computing modular inverses (which might also be useful if we want to make the trial division table smaller, etc.), but I'm running into a couple of issues trying to refactor
|
I thought I had a trait-based solution for this, but it doesn't work out: I guess the way forward is to accept a little bit of code duplication, as it seems like a macro-based solution would not be too good here (even worse maintainability) |
Oh? |
I think we need to add I think we can apply size-specific optimizations by playing around with the traits a bit more. |
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.
I don’t really have any other changes. I guess just double-check that my casts/conversions are correct if you haven’t already.
@Arcterus Thanks for the feedback, I'll look later this week; maybe tomorrow if I feel up for it |
Yes, I think I can do the optimization I had in mind in a pretty straightforward way |
|
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.
These comments are rather nit-picky. After you address these, I think the code is good to go.
src/uu/factor/src/numeric.rs
Outdated
fn reduce(&self, x: T::Double) -> T { | ||
let t_bits = T::zero().count_zeros() as usize; | ||
|
||
debug_assert!(x < (self.n.as_double()) << t_bits); |
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.
I feel like as_double()
will confuse people coming from more C-like languages. It sounds like you are converting the value into a f64
.
The name could be changed to as_dint()
or something like that (although I don’t really like that name either). Another alternative is to find a different name for DoubleInt
, although I can’t think of a good name at the moment.
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.
Eh; I had the same thought, but I really couldn't find a better name (except maybe spelling out “double-width” in full, but that doesn't seem more helpful).
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.
@Arcterus Were you able to think of a more-helpful name? I wasn't :(
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.
From my review of the code, it looks like DoubleInt
is being used as a dual-precision integer, generally to avoid overflow issues. Correct?
If so, how about DualPrecisionInt
(or maybe VariablePrecisionInt
)?
That might lead to as_high_precision()
and from_high_precision()
trait functions and annotations like T::HighPrecision::one()
.
More verbose, but possibly more informative to readers.
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.
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.
PS: Also documented the type in 33a1c44
The build failure is due to one task, Style (macos-latest, macos), failing to install the Rust toolchain. |
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.
Detected by the testsuite improvement just prior.
@Arcterus Sorry, this got a bit out of hand, but I ended up doing some serious improvements to the testsuite to be confident that there were no bugs left, ended up finding a couple more issues in |
Also, this is currently twice as fast as |
Bravo for the coverage |
Thanks regardless 💜 |
@nbraud could you please fix the conflicts? sorry thanks |
No need to apologise, I already had a local commit to resolve the merge conflict, I just needed to know which PR it would go to :) |
Tests are passing. |
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.
I think that a bit of name refactoring for DoubleInt
, ..., etc. would help improve the code clarity.
src/uu/factor/src/numeric.rs
Outdated
fn reduce(&self, x: T::Double) -> T { | ||
let t_bits = T::zero().count_zeros() as usize; | ||
|
||
debug_assert!(x < (self.n.as_double()) << t_bits); |
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.
From my review of the code, it looks like DoubleInt
is being used as a dual-precision integer, generally to avoid overflow issues. Correct?
If so, how about DualPrecisionInt
(or maybe VariablePrecisionInt
)?
That might lead to as_high_precision()
and from_high_precision()
trait functions and annotations like T::HighPrecision::one()
.
More verbose, but possibly more informative to readers.
- `DoubleInt::Double` renamed to `DoubleWidth` - `{as,from}_double()` renamed to `{as,from}_double_width()`. This should hopefully clarify that this is not a “double precision” floating-point type, but an integer type with a larger range (used for storing intermediate results, typ. from a multiplication)
It was unused, the debug assertions only need `to_u128`.
@rivy Thanks! |
Merged. |
factor: Refactor and improve performance (plus a few bug fixes)
Twice as fast as
master