Implement Pow for BigUint and BigInt #54

thomwiggers · 2018-07-04T12:59:14Z

Implements #29

cuviper

Thanks for the PR!

cuviper · 2018-07-06T00:27:00Z

src/biguint.rs

+            }
+        }
+
+        impl<'a> pow::Pow<$signed> for BigUint {


Let's not do signed exponents here at all. Notice that for primitives, we only have signed Pow for the floating point types.

cuviper · 2018-07-06T00:37:17Z

src/biguint.rs

+            #[inline]
+            fn pow(self, exp: $unsigned) -> Self::Output {
+                // square and multiply from num_traits::pow
+                pow::pow(self.clone(), exp as usize)


This can be faster if done directly, because a lot of value cloning can be avoided. We can't make that change in num_traits::pow yet for compatibility, but for some history see rust-num/num-traits#18, and the implementation in rust-num/num#334. That was only merged on the next branch, and never ultimately used.

Feel free to copy that code and adapt it here. It might as well work with the exponent type directly too, so you don't have to worry about exp as usize overflowing for any type, even u128. Not that it's realistic to use such exponents, but we can at least try to do the right thing if we support it at all.

Doing this directly also immediately deals with the u128 implementation that was still FIXME.

cuviper · 2018-07-06T00:38:04Z

src/biguint.rs

+            #[inline]
+            fn pow(self, exp: $unsigned) -> Self::Output {
+                // square and multiply from num_traits::pow
+                pow::pow(self, exp as usize)


Given a direct implementation by-reference, I would just forward to that from here.

For completeness, we should also have Pow<&'a $unsigned> for BigUint and Pow<&'a $unsigned> for &'b BigUint.

The exponents we implement are all Copy so I don't see much value for a reference – one which we'll immediately clone anyway, as we need to mutate it.

I also got rid of the implementation on BigUint and BigInt directly, the references should suffice.

cuviper · 2018-07-06T00:45:04Z

src/bigint.rs

@@ -66,6 +66,24 @@ impl Mul<Sign> for Sign {
    }
 }

+impl<T: PrimInt + BitAnd<Output=T>> Pow<T> for Sign {


BitAnd is already implied by PrimInt.

I'm a little hesitant to have this impl at all, but I guess it's in line with Mul and Neg for Sign. It's a little weird that "0" NoSign to the 0th power should be Plus, as the closest thing to a "1", but whatever.

cuviper · 2018-07-06T01:00:07Z

src/bigint.rs

+        if self != Minus {
+            self
+        } else if other == T::zero() {
+            Plus


The check should use other.is_zero(), and this case should be first, because we also need Plus for NoSign.pow(0).

cuviper · 2018-07-06T01:02:06Z

src/bigint.rs

+            self
+        } else if other == T::zero() {
+            Plus
+        } else if other & T::one() == T::one() {


Slightly better to use (other & T::one()).is_one() -- or if you switch to T: Integer, it's just other.is_odd().

cuviper · 2018-07-06T01:11:11Z

src/bigint.rs

@@ -811,6 +829,28 @@ impl Signed for BigInt {
    }
 }

+impl<T: PrimInt> Pow<T> for BigInt
+    where BigUint: Pow<T, Output=BigUint>


Let's have exponent references too, as I mentioned on BigUint.

I think it's technically a breaking change to do this generically, as someone could already have an impl, however silly, like:

struct Foo(usize); impl Pow<Foo> for BigInt { type Output = BigInt; fn pow(self, exp: Foo) -> BigInt { num_traits::pow(self, exp.0) } }

Let's just avoid that question by implementing it in macros like BigUint.

thomwiggers

Thanks for the useful feedback, it was helpful. I've incorporated it.

thomwiggers · 2018-07-09T15:07:19Z

src/biguint.rs

+            #[inline]
+            fn pow(self, exp: $unsigned) -> Self::Output {
+                // square and multiply from num_traits::pow
+                pow::pow(self.clone(), exp as usize)


Doing this directly also immediately deals with the u128 implementation that was still FIXME.

thomwiggers · 2018-07-09T15:08:53Z

src/biguint.rs

+            #[inline]
+            fn pow(self, exp: $unsigned) -> Self::Output {
+                // square and multiply from num_traits::pow
+                pow::pow(self, exp as usize)


I also got rid of the implementation on BigUint and BigInt directly, the references should suffice.

cuviper

I do want all the ref/value combinations implemented, even though the one you have is rightly the primary way to do it. The rest can just forward to that.

It's useful for generic code to have all of these cases covered -- note how the primitive integers also have all their ref/value implementations of the standard ops, even though they're trivially Copy types.

cuviper · 2018-07-12T21:59:01Z

src/biguint.rs

+
+                if exp == 1 { return base; }
+
+                let mut acc = self.clone();


This should be base.clone(). None of your tests reached this, because they're all power-of-two exponents that hit if exp == 1 above.

Err, actually you do have a test for pow(3) -- but since that has no trailing zeros, base and self are still the same. Try an exponent like 10 to see the failure I'm talking about.

oops, I missed that when copying the pow implementation from next.

Added a few tests as well.

cuviper · 2018-07-12T22:02:08Z

tests/bigint.rs

+    let two = 2u32.to_bigint().unwrap();
+    let minus_two = -2i32.to_bigint().unwrap();
+    let four = 4u32.to_bigint().unwrap();
+    let eight = 8u32.to_bigint().unwrap();


These can use From like BigInt::from(8u32) nowadays -- although I know many other tests are still doing it the hard way too. (We ought to clean that up...)

cuviper · 2018-07-12T22:03:20Z

src/biguint.rs

+                // square and multiply from num_traits::pow in the next branch
+                let mut base = self.clone();
+
+                if exp == 0 { return BigUint::one(); }


Might as well check this first, to potentially avoid a useless clone.

Good point.

cuviper · 2018-07-12T22:06:49Z

src/biguint.rs

+
+            #[inline]
+            fn pow(self, mut exp: $unsigned) -> Self::Output {
+                // square and multiply from num_traits::pow in the next branch


Let's not immortalize the next branch in comments -- that stuff is obsolete. If you're trying to give credit, then a note in your commit message is enough.

cuviper · 2018-07-12T22:09:04Z

src/bigint.rs

@@ -66,6 +66,24 @@ impl Mul<Sign> for Sign {
    }
 }

+impl<'a, T: Integer> Pow<&'a T> for Sign {


I'm still hesitating whether we really want a public impl for this. It's kind of silly to have a bunch of traits implemented for Sign. (An alternative is to just have a private fn to get the new sign.)

Private fn ✔️

I'd have expected the entire Sign trait private by the way, but there's probably some good reason for it.

It's an enum Sign, not a trait, and it's a public argument for some of the constructors. We have to indicate the sign somehow, since we're not using 2's complement, so an enum is relatively clean. It usually stays out of the way. :)

56: Implement Roots for BigInt and BigUint r=cuviper a=mancabizjak Supersedes #51 . Since there is now a `Roots` trait with `sqrt`, `cbrt` and `nth_root` methods in the `num-integer` crate, this PR implements it for `BigInt` and `BigUint` types. I also added inherent methods on both types to allow the users access to all these functions without having to import `Roots`. PS: `nth_root` currently uses `num_traits::pow`. Should we perhaps wait for #54 to get merged, and then replace the call to use the new `pow::Pow` implementation on `BigUint`? Co-authored-by: Manca Bizjak <manca.bizjak@xlab.si>

thomwiggers

I've addressed the feedback.

Pow is now implemented for &u8 and u8 (and the rest of the primitive integer types).

thomwiggers · 2018-07-19T14:45:54Z

src/bigint.rs

@@ -66,6 +66,24 @@ impl Mul<Sign> for Sign {
    }
 }

+impl<'a, T: Integer> Pow<&'a T> for Sign {


Private fn ✔️

thomwiggers · 2018-07-19T14:48:48Z

src/biguint.rs

+
+                if exp == 1 { return base; }
+
+                let mut acc = self.clone();


oops, I missed that when copying the pow implementation from next.

thomwiggers · 2018-07-19T14:48:55Z

src/biguint.rs

+
+                if exp == 1 { return base; }
+
+                let mut acc = self.clone();


Added a few tests as well.

thomwiggers · 2018-07-19T14:49:03Z

tests/bigint.rs

+    let two = 2u32.to_bigint().unwrap();
+    let minus_two = -2i32.to_bigint().unwrap();
+    let four = 4u32.to_bigint().unwrap();
+    let eight = 8u32.to_bigint().unwrap();


thomwiggers · 2018-07-19T14:50:02Z

src/bigint.rs

@@ -66,6 +66,24 @@ impl Mul<Sign> for Sign {
    }
 }

+impl<'a, T: Integer> Pow<&'a T> for Sign {


I'd have expected the entire Sign trait private by the way, but there's probably some good reason for it.

thomwiggers · 2018-07-19T14:50:13Z

src/biguint.rs

+                // square and multiply from num_traits::pow in the next branch
+                let mut base = self.clone();
+
+                if exp == 0 { return BigUint::one(); }


Good point.

thomwiggers · 2018-07-19T14:57:06Z

src/biguint.rs

+
+            #[inline]
+            fn pow(self, mut exp: $unsigned) -> Self::Output {
+                // square and multiply from num_traits::pow in the next branch


Build succeeded

continuous-integration/travis-ci/push

thomwiggers · 2018-08-04T07:55:03Z

Yeah, that's something I've run into as well. I probably meant to comment that but forgot :p With some luck will the compiler take care of the overhead of the ref where it could've been by value. Josh Stone <notifications@github.com> schreef op August 4, 2018 12:12:04 AM UTC:

…

cuviper commented on this pull request. > @@ -434,6 +434,55 @@ impl One for BigUint { impl Unsigned for BigUint {} +macro_rules! pow_impl { + ($T:ty) => { + impl<'a> Pow<$T> for &'a BigUint { Just for posterity: I was going to ask you to add `for BigUint` (by value) too, but I tried it myself first. That makes it really annoying when you do have a bigint value, as the `pow` method will then try to move ownership. You'd have to manually reference `(&i).pow(exp)` to avoid this and get the by-ref method, which is really bad for ergonomics. So if anyone asks why we don't have that `impl`, here's the reason. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #54 (review)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

cuviper requested changes Jul 6, 2018

View reviewed changes

thomwiggers commented Jul 9, 2018

View reviewed changes

mancabizjak mentioned this pull request Jul 11, 2018

Implement Roots for BigInt and BigUint #56

Merged

cuviper requested changes Jul 12, 2018

View reviewed changes

thomwiggers commented Jul 19, 2018

View reviewed changes

thomwiggers added 9 commits July 19, 2018 17:02

Support BigUint.pow() for primitives

cfe8ffb

Implement pow for BigInt

136d7d9

Implement Pow directly on references

3f79349

Address feedback

9009cf8

Also allow BigInt.pow(&$type)

ab3594f

also use BigInt::from

d3ef151

Get rid of num_traits::pow usage

5fe8a5f

See also #56

Avoid 'unsigned' and 'type' as macro variable

de5b78c

It seems to break R1.15

Fix benchmark

03e01cb

cuviper reviewed Aug 4, 2018

View reviewed changes

cuviper approved these changes Aug 4, 2018

View reviewed changes

bors bot added a commit that referenced this pull request Aug 4, 2018

Merge #54

2e19875

54: Implement Pow for BigUint and BigInt r=cuviper a=thomwiggers Implements #29 Co-authored-by: Thom Wiggers <thom@thomwiggers.nl>

bors bot merged commit 03e01cb into rust-num:master Aug 4, 2018

cuviper mentioned this pull request Jan 23, 2024

Add powi method for Sign enum #289

Open

@@ @@ -434,6 +434,55 @@ impl One for BigUint { @@
               impl Unsigned for BigUint {}
+              macro_rules! pow_impl {
+                  ($T:ty) => {
+                      impl<'a> Pow<$T> for &'a BigUint {

Implement Pow for BigUint and BigInt #54

Implement Pow for BigUint and BigInt #54

Conversation

thomwiggers commented Jul 4, 2018

cuviper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomwiggers Jul 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomwiggers left a comment

Choose a reason for hiding this comment

thomwiggers Jul 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomwiggers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper commented Aug 4, 2018

bors bot commented Aug 4, 2018

Build succeeded

thomwiggers commented Aug 4, 2018 via email

thomwiggers Jul 9, 2018 •

edited

thomwiggers Jul 9, 2018 •

edited