Implement Roots for BigInt and BigUint #56

mancabizjak · 2018-07-11T10:11:34Z

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?

This commit implements num-integer::Roots trait for BigInt and BigUint types, and also adds sqrt, cbrt, nth_root as inherent methods to allow access to them without importing Roots trait. For each type tests were added as submodules in the roots test module. Signed-off-by: Manca Bizjak <manca.bizjak@xlab.si>

cuviper

Requested a few small changes.

The new Pow ought to be faster, since it can do ops by reference (i.e. less cloning), but we could just make that improvement in a followup.

cuviper · 2018-07-12T22:22:53Z

src/biguint.rs

+        }
+
+        let n = n as usize;
+        let n_min_1 = (n as usize) - 1;


Don't need to cast n as usize twice.

cuviper · 2018-07-12T22:27:34Z

src/biguint.rs

+
+    /// Returns the truncated principal square root of `self` --
+    /// see [Roots::sqrt](Roots::sqrt).
+    // struct.BigInt.html#trait.Roots


These links don't look right -- did you forget to complete this?

Eh, forgot to remove.

cuviper · 2018-07-12T22:31:21Z

src/biguint.rs

 }

+impl Roots for BigUint {
+    fn nth_root(&self, n: u32) -> Self {


I think it would probably be valuable to have dedicated sqrt and cbrt implementations -- did you try it? Maybe benchmarks would prove me wrong, but we can also wait until later.

Here are the benchmark results:

test roots_cbrt ... bench: 48,670 ns/iter (+/- 1,322) test roots_cbrt_nth_root_3 ... bench: 52,285 ns/iter (+/- 1,541) test roots_sqrt ... bench: 53,752 ns/iter (+/- 4,954) test roots_sqrt_nth_root_2 ... bench: 57,622 ns/iter (+/- 1,585)

roots_cbrt and roots_sqrt test the optimized versions - I took sqrt from #51 and adjusted it for cbrt. The other two benchmarks just called nth_root(2) and nth_root(3) - of course, for the sake of benchmark, I didn't forward the calls to nth_root with n=2 and n=3 to the optimized sqrt and cbrt, but I'll definitely do that, since I see this is how it's done in impls for primitive types as well.

I would prefer to incorporate the dedicated implementations in this PR, although I'm struggling a little since I'm not particularly fond of having parts of nth_root, sqrt, cbrt replicated in all the methods. But I'm not too sure what (if anything) would be best to do about it 🙂

cuviper · 2018-07-12T22:34:22Z

tests/roots.rs

+    use std::str::FromStr;
+
+    fn check(x: i32, n: u32, expected: i32) {
+        let big_x: BigUint = FromPrimitive::from_i32(x).unwrap();


If you use an unsigned type instead of i32, then From makes this simpler -- BigUint::from(x).

cuviper · 2018-07-12T22:35:01Z

tests/roots.rs

+    use num_traits::FromPrimitive;
+
+    fn check(x: i32, n: u32, expected: i32) {
+        let big_x: BigInt = FromPrimitive::from_i32(x).unwrap();


Similarly, BigInt::from(x) should be fine.

cuviper · 2018-07-12T22:39:32Z

benches/bigint.rs

+    // hugely impacts the performance of nth_root due to exponentiation to
+    // the power of n-1. Using very large values for n is also not very realistic,
+    // and any n > x's bit size produces 1 as a result anyway.
+    let n: u8 = rng.gen();


This is from a seeded RNG, so what value do you get? I think you might as well just pick a reasonable n directly. Random x is still fine.

This commit overrides default implementations of Roots::sqrt and Roots::cbrt for BigInt and BigUint with optimized ones. It also improves tests and resolves minor inconsistencies. Signed-off-by: Manca Bizjak <manca.bizjak@xlab.si>

mancabizjak · 2018-07-13T11:05:16Z

Thanks for the review! I overrided sqrt and cbrt methods with versions slightly more performant than their default implementations with nth_root. Recurring checks for trivial cases are now a bit more compact.

In addition, after glancing at the test code for implementations of Roots on PrimInts, I felt that the test code was not thorough enough, so I improved it a little.

I also fixed some inconsistencies in comments and made panic messages a bit clearer.

cuviper

I understand your concern about duplicating similar code, but I think the performance gain is worth it, and it's not that much code.

cuviper · 2018-07-17T00:34:13Z

src/bigint.rs

+
+    /// Returns the truncated principal square root of `self` --
+    /// see [Roots::sqrt](Roots::sqrt).
+    // struct.BigInt.html#trait.Roots


Another stray link? And I think all of these links like [Roots::sqrt](Roots::sqrt) won't actually work, AFAICS.

When I run cargo doc --no-deps --open and navigate to the page of BigInt or BigUint structs, paths of the form Roots::sqrt are rendered as links to https://docs.rs/num-integer/0.1/num_integer/trait.Roots.html#method.sqrt - I'm a little confused, is this not what we want? I was merely following this RFC, but I could be missing something.

Ah, it seems that works on nightly, but not beta or stable rustc yet. We should make those explicit for now.

cuviper · 2018-07-17T00:41:16Z

src/biguint.rs

+        let n_min_1 = n - 1;
+
+        let bit_len = self.len() * big_digit::BITS;
+        let guess = BigUint::one() << (bit_len/n + 1);


The bit_len can be just self.bits(). That should also get a better initial guess when the most-significant word is less "full".

Signed-off-by: Manca Bizjak <manca.bizjak@xlab.si>

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

Thanks!

bors r+

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>

bors · 2018-07-19T00:09:32Z

Build succeeded

continuous-integration/travis-ci/push

mancabizjak mentioned this pull request Jul 11, 2018

Implement sqrt for BigInt and BigUint #51

Closed

mancabizjak force-pushed the impl-roots branch from ce04a0f to 1f25906 Compare July 11, 2018 10:15

cuviper requested changes Jul 12, 2018

View reviewed changes

Implement optimized sqrt, cbrt methods

2b473e9

This commit overrides default implementations of Roots::sqrt and Roots::cbrt for BigInt and BigUint with optimized ones. It also improves tests and resolves minor inconsistencies. Signed-off-by: Manca Bizjak <manca.bizjak@xlab.si>

mancabizjak force-pushed the impl-roots branch from 990a520 to 2b473e9 Compare July 13, 2018 10:53

cuviper reviewed Jul 17, 2018

View reviewed changes

mancabizjak added 2 commits July 17, 2018 15:06

Remove stale doc path, use self.bits()

bbe7b18

Signed-off-by: Manca Bizjak <manca.bizjak@xlab.si>

Update links to Roots trait methods

1d45ca9

Signed-off-by: Manca Bizjak <manca.bizjak@xlab.si>

cuviper approved these changes Jul 18, 2018

View reviewed changes

bors bot merged commit 1d45ca9 into rust-num:master Jul 19, 2018

mancabizjak deleted the impl-roots branch July 30, 2018 11:15

Implement Roots for BigInt and BigUint #56

Implement Roots for BigInt and BigUint #56

Uh oh!

Conversation

mancabizjak commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cuviper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mancabizjak commented Jul 13, 2018

Uh oh!

cuviper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cuviper commented Jul 19, 2018

Uh oh!

bors bot commented Jul 19, 2018

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mancabizjak commented Jul 11, 2018 •

edited

Loading