Skip to content
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

Add new function gcd_lcm for calculating the gcd and lcm simultaneously (performance) #12

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,21 @@ pub trait Integer: Sized + Num + PartialOrd + Ord + Eq {
/// ~~~
fn lcm(&self, other: &Self) -> Self;

/// Greatest Common Divisor (GCD) and
/// Lowest Common Multiple (LCM) simultaneously.
///
/// More efficient than calling `gcd` and `lcm`
/// individually for identical inputs.
///
/// # Examples
///
/// ~~~
/// # use num_integer::Integer;
/// assert_eq!(10.gcd_lcm(&4), (2, 20));
/// assert_eq!(8.gcd_lcm(&9), (1, 72));
/// ~~~
fn gcd_lcm(&self, other: &Self) -> (Self, Self);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that it's a breaking change to add a trait method without a default implementation. For instance, even num-bigint would fail to build with this until it adds the new method.

I think it should be straightforward to implement this generically, although it probably needs where Self: Clone to do the math by-value. When we do implement this for num-bigint, we can do so without those external clones, so overall this should be fine.


/// Deprecated, use `is_multiple_of` instead.
fn divides(&self, other: &Self) -> bool;

Expand Down Expand Up @@ -206,6 +221,13 @@ pub fn lcm<T: Integer>(x: T, y: T) -> T {
x.lcm(&y)
}

/// Calculates the Greatest Common Divisor (GCD) and
/// Lowest Common Multiple (LCM) of the number and `other`.
#[inline(always)]
pub fn gcd_lcm<T: Integer>(x: T, y: T) -> (T, T) {
x.gcd_lcm(&y)
}

macro_rules! impl_integer_for_isize {
($T:ty, $test_mod:ident) => (
impl Integer for $T {
Expand Down Expand Up @@ -297,6 +319,16 @@ macro_rules! impl_integer_for_isize {
(*self * (*other / self.gcd(other))).abs()
}

/// Calculates the Greatest Common Divisor (GCD) and
/// Lowest Common Multiple (LCM) of the number and `other`.
#[inline]
fn gcd_lcm(&self, other: &Self) -> (Self, Self) {
let gcd = self.gcd(other);
// should not have to recalculate abs
let lcm = (*self * (*other / gcd)).abs();
(gcd, lcm)
}

/// Deprecated, use `is_multiple_of` instead.
#[inline]
fn divides(&self, other: &Self) -> bool {
Expand Down Expand Up @@ -475,6 +507,15 @@ macro_rules! impl_integer_for_isize {
assert_eq!((11 as $T).lcm(&5), 55 as $T);
}

#[test]
fn test_gcd_lcm() {
for i in 1..=255 {
for j in 0..=255 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be fine to use a plain range for compatibility.

Maybe I'm missing something, but I'm surprised that this didn't warn about 255 being out of range for i8. But anyway, if you want to be exhaustive, perhaps we should use negative values too with -128..127.

I also notice that you avoid lcm(0, 0), but I think this is actually a bug in our existing implementation. We would get a divide-by-zero since gcd(0, 0) = 0, but Wolfram Alpha says lcm(0, 0) = 0 too.

assert_eq!(i.gcd_lcm(&j), (i.gcd(&j), i.lcm(&j)));
}
}
}

#[test]
fn test_even() {
assert_eq!((-4 as $T).is_even(), true);
Expand Down Expand Up @@ -560,6 +601,15 @@ macro_rules! impl_integer_for_usize {
*self * (*other / self.gcd(other))
}

/// Calculates the Greatest Common Divisor (GCD) and
/// Lowest Common Multiple (LCM) of the number and `other`.
#[inline]
fn gcd_lcm(&self, other: &Self) -> (Self, Self) {
let gcd = self.gcd(other);
let lcm = *self * (*other / gcd);
(gcd, lcm)
}

/// Deprecated, use `is_multiple_of` instead.
#[inline]
fn divides(&self, other: &Self) -> bool {
Expand Down Expand Up @@ -653,6 +703,15 @@ macro_rules! impl_integer_for_usize {
assert_eq!((15 as $T).lcm(&17), 255 as $T);
}

#[test]
fn test_gcd_lcm() {
for i in 1..=255 {
for j in 0..=255 {
assert_eq!(i.gcd_lcm(&j), (i.gcd(&j), i.lcm(&j)));
}
}
}

#[test]
fn test_is_multiple_of() {
assert!((6 as $T).is_multiple_of(&(6 as $T)));
Expand Down