Added is_probable_prime using Rabin Miller testing #16

KingAkeem · 2018-01-04T23:47:45Z

is_probable_prime is a public member method of BigInt that uses the Rabin Miller primality test, a user-selected number ,passed in as a parameter named "certainty", of times to determine if a BigInt is prime or not.

codecov · 2018-01-05T00:30:14Z

Codecov Report

Merging #16 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #16   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     12    -1     
  Lines         536    533    -3     
=====================================
- Hits          536    533    -3

Impacted Files	Coverage Δ
include/BigInt.hpp	`100% <ø> (ø)`	⬆️
include/functions/math.hpp	`100% <100%> (ø)`	⬆️
include/functions/utility.hpp	`100% <0%> (ø)`	⬆️
include/operators/binary_arithmetic.hpp	`100% <0%> (ø)`	⬆️
include/functions/random.hpp

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f580686...4bec71a. Read the comment docs.

faheel

gcd() has been added so please merge the changes from upstream.
Add spaces on both sides of all binary operators.
See the coverage report to see which lines haven't been hit while running the tests, and add a few such cases that would execute those lines.
Use or and and instead of || and && respectively.

faheel · 2018-01-05T11:50:20Z

include/BigInt.hpp

@@ -99,6 +99,9 @@ class BigInt {
        int to_int() const;
        long to_long() const;
        long long to_long_long() const;
+
+        // Math functions:
+        bool is_probable_prime(size_t certainty);


Add the const modifier at the end as in the methods above.

faheel · 2018-01-05T11:51:01Z

include/functions/math.hpp

@@ -125,4 +124,60 @@ BigInt sqrt(const BigInt& num) {
    return sqrt_current;
 }

+/*
+   is_portable_prime


faheel · 2018-01-05T11:57:23Z

include/functions/math.hpp

+    BigInt temp_d = d;
+    BigInt temp2_d = d;
+    BigInt TWO = 2;
+    for (;0<certainty;certainty--) {


You can change this to a while loop.

while (certainty-- > 0) { ... }

KingAkeem · 2018-01-05T17:34:31Z

Just added the changes, thanks for the feedback

faheel

Sorry for the late review 😅

faheel · 2018-01-17T23:37:22Z

include/functions/math.hpp

+        while (temp2_d > 0) {
+            // Getting last character from BigInt and checking if even or odd
+            std::string temp_s = temp2_d.to_string();
+            if ((int) temp_s.back() & 1) {


The following is easier to read and is also more efficient since the string returned by to_string() is temporary and is automatically destroyed:

int last_digit = temp2_d.to_string().back() - '0'; if (last_digit % 2) ...

We could use this version but there will be a slit performance hit.

The "ModVersion" one is slightly faster. Is it the one with the suggested change?

Sorry that's a little confusing. Here's a clearer image.

I found the following to be the fastest of the 4 ways that I tested:

int last_digit = y.to_string().back() - '0'; if (last_digit & 1) ...

That should work, the operation that was causing the most performance loss was the modulo so we should be able to use this code. I'm about to test it now.

It's still slightly slower on my machine.

Dropping the - '0' part should also work correctly and will be slightly faster.

The results have been very strange. Sometimes it runs faster, sometimes it's the same speed and sometimes it's slower. I'm going to use a profiler to measure the performance because I don't trust the time command to be completely accurate.

faheel · 2018-01-17T23:38:25Z

include/functions/math.hpp

+        // Corner cases ensure num > 4
+        BigInt a =  TWO + BigInt(rand()) % (num - 4);
+        BigInt res = 1;
+        BigInt x = a % num;


Try to use better names for a and x.

faheel · 2018-01-17T23:42:34Z

include/functions/math.hpp

+    }
+
+    BigInt temp_d = d;
+    BigInt temp2_d = d;


Can you come up with better names for d, temp_d and temp2_d?

I can't really think of really creative names for them but I've put more comments to explain their function.

faheel · 2018-01-18T00:51:47Z

include/functions/math.hpp

+    if (num <= 1 or num == 4) return false;
+    if (num <= 3) return true;
+
+    // Find r such that n = 2^d * r + 1 for some r >= 1


Is n = num and r = res? If so, then please make sure that the names in the formula and in the code are the same in order to explain the code better.

faheel · 2018-01-18T00:58:36Z

include/functions/math.hpp

+ */
+
+bool BigInt::is_probable_prime(size_t certainty) const {
+    BigInt num = this->value;


This will set num to the absolute value of this object (same as num = abs(*this)). Is that desired behaviour when a number is negative? (I'm just confirming since I'm not sure myself.)

From Wolfram MathWorld:

A prime number (or prime integer, often simply called a "prime" for short) is a positive integer p>1 that has no positive integer divisors other than 1 and p itself.

Negative integers are therefore not prime. So you should change this line to:

BigInt num = *this;

Alright, thanks I've fixed that issue

faheel · 2018-01-18T01:11:49Z

test/functions/math.test.cpp

+    num = "4702877858580764415667235078667510929270158248348";
+    REQUIRE(num.is_probable_prime(1) == false);
+    num = "378348910233465647859184421334615532543749747185321634086219";
+    REQUIRE(num.is_probable_prime(1) == true);


It would be good to have checks for even larger numbers, and with more variation in the value of certainty.
Also, please add a few cases for negative numbers too.

Using bigger numbers and raising certainty will raise the time dramatically but I will implement both.

Using bigger numbers and raising certainty will raise the time dramatically

Yes, that's why we're testing it with bigger values - to check that it shouldn't take too long to work out the result for much larger numbers.

Understood, because of the way the algorithm works with certainty. It's going to take a long time unless we optimize the operations for BigInt. I agree with you though, I believed that it shouldn't take too long to work out result for much larger numbers. How would you like to tackle the problem?

faheel · 2018-01-18T15:43:58Z

include/functions/math.hpp

+/*    
+    Gcd (BigInt, BigInt) :
+    --------------------
+


Seems like these 4 lines were added my mistake while resolving merge conflicts.

I'll make sure to get that done.

Are you sure that you want me to remove this comment? It's above the function definition for BigInt gcd(const BigInt &num1, const BigInt &num2) would you like me to delete this function also?

This comment is extra. If you look further, you'll see the comment for gcd(BigInt, BigInt). So just remove these 4 lines.

Understood, thanks

faheel · 2018-01-18T15:45:14Z

test/functions/math.test.cpp

+    Naive gcd for testing purpose.
+*/
+
+long long naive_gcd(long long a, long long b){


Lines 290-311 were replaced by gcd(long long, long long) that follows, so please remove them.

I'll also get this done.

faheel added the functions: math Math functions label Jan 5, 2018

faheel requested changes Jan 5, 2018

View reviewed changes

faheel requested changes Jan 18, 2018

View reviewed changes

faheel reviewed Jan 18, 2018

View reviewed changes

KingAkeem closed this May 10, 2018

KingAkeem force-pushed the master branch from 4bec71a to f580686 Compare May 10, 2018 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added is_probable_prime using Rabin Miller testing #16

Added is_probable_prime using Rabin Miller testing #16

KingAkeem commented Jan 4, 2018

codecov bot commented Jan 5, 2018 •

edited

Loading

faheel left a comment •

edited

Loading

faheel Jan 5, 2018

faheel Jan 5, 2018

faheel Jan 5, 2018

KingAkeem commented Jan 5, 2018

faheel left a comment

faheel Jan 17, 2018

KingAkeem Jan 18, 2018

faheel Jan 18, 2018

KingAkeem Jan 18, 2018

faheel Jan 18, 2018

KingAkeem Jan 18, 2018

KingAkeem Jan 18, 2018

faheel Jan 18, 2018 •

edited

Loading

KingAkeem Jan 18, 2018

faheel Jan 17, 2018

faheel Jan 17, 2018

KingAkeem Jan 18, 2018

faheel Jan 18, 2018

faheel Jan 18, 2018

faheel Jan 18, 2018 •

edited

Loading

KingAkeem Jan 18, 2018

faheel Jan 18, 2018 •

edited

Loading

KingAkeem Jan 18, 2018

faheel Jan 18, 2018

KingAkeem Jan 18, 2018

faheel Jan 18, 2018

KingAkeem Jan 18, 2018

KingAkeem Jan 18, 2018

faheel Jan 18, 2018

KingAkeem Jan 18, 2018

faheel Jan 18, 2018

KingAkeem Jan 18, 2018

Added is_probable_prime using Rabin Miller testing #16

Added is_probable_prime using Rabin Miller testing #16

Conversation

KingAkeem commented Jan 4, 2018

codecov bot commented Jan 5, 2018 • edited Loading

Codecov Report

faheel left a comment • edited Loading

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

KingAkeem commented Jan 5, 2018

faheel 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

faheel Jan 18, 2018 • edited Loading

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

faheel Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faheel Jan 18, 2018 • edited Loading

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

codecov bot commented Jan 5, 2018 •

edited

Loading

faheel left a comment •

edited

Loading

faheel Jan 18, 2018 •

edited

Loading

faheel Jan 18, 2018 •

edited

Loading

faheel Jan 18, 2018 •

edited

Loading