Added Fast Power in Number Theory #76

ahmad307 · 2017-07-05T23:22:11Z

This is my first PR, please tell me if there are any issues to be fixed.
Relates to issue #75.

faheel

It would be great if you also add tests for this, similar to the ones found here. Let me know if you need more information regarding it.

faheel · 2017-07-06T05:11:38Z

NumberTheory/FastPower.cpp

+#include <iostream>
+
+using namespace std;
+typedef long long ll;


Use unsigned long long for double the range.

faheel · 2017-07-06T05:12:45Z

NumberTheory/FastPower.cpp

+typedef long long ll;
+
+//Function that returns x raised to the power of y
+ll fastPower (ll x,ll y)


Please use more meaningful variable names like base and exponent instead of just x and y.
Also, rename the function to fast_power to adhere to the guidelines.

faheel · 2017-07-06T05:19:10Z

NumberTheory/FastPower.cpp

@@ -0,0 +1,47 @@
+/*
+<Fast Power>


Please remove the angle brackets (< and >) and indent the block by one level (4 spaces).

faheel · 2017-07-06T05:20:27Z

NumberTheory/FastPower.cpp

+
+    cin>>base>>power;
+
+    cout<<fastPower(base,power);


Add a trailing "\n" so that the prompt on terminals is displayed in the next line.

faheel · 2017-07-06T05:21:52Z

NumberTheory/FastPower.cpp

+
+Space Complexity
+------------------
+O(log(N)) where N is the power the number is raised to.


~~where N is the power the number is raised to~~
More simply, where N is the exponent.

faheel · 2017-07-06T05:23:29Z

NumberTheory/FastPower.cpp

+<Fast Power>
+--------------
+Fast Power is an optimized algorithm to compute exponentiation in a short time.
+


It would be helpful for others if you could also mention in 1 or 2 lines more what the algorithm does.

ahmad307 · 2017-07-06T09:57:33Z

Regarding adding tests. Shall I add some sample tests commented?
Or shall I make them this way:

TEST_CASE("Base cases", "[fast_power]") {
   REQUIRE(fast_power(2,2) == 4);
   REQUIRE(fast_power(2,4) == 16);
}

faheel · 2017-07-06T10:11:01Z

Like the example you gave.

I have updated the file with the required changes alongside adding sample tests.

faheel · 2017-07-06T22:56:39Z

NumberTheory/FastPower.cpp

+    //Sample Tests
+    TEST_CASE("Base cases", "[fast_power]") {
+        REQUIRE(fast_power(2,2) == 4);
+        REQUIRE(fast_power(2,4) == 16);


I think you didn't exactly understand how to add tests, but no worries 😄! Read the second comment in issue #41 to know the exact steps in detail.

Removed test cases from the source file.

ahmad307 · 2017-07-07T23:33:48Z

I have now added a test file for fastPower.cpp, and edited fastPower.cpp to be compatible.

faheel

Well done on adding the tests! 👍
However there are a few issues I have mentioned in the review. Also, the f in the name of the test file should be capital.

faheel · 2017-07-08T06:42:36Z

Test/NumberTheory/fastPower.test.cpp

+
+TEST_CASE("Overflow cases", "[fast_power]") {
+    REQUIRE(fast_power(2,100) == 0);
+    REQUIRE(fast_power(10,99) == 0);


fast_power(2, 100) should be 68719476736 (see this) and fast_power(10, 99) should be 4440381706574496940U (see this). Suffix U is for unsigned.

I think you meant fast_power(2 ,100) in the first one.
Anyway, regarding large numbers, to fix the output being 0 we can output them modulo a large number such as 1000000000007 , but not a huge number like 2^64. Where fast_power(2, 100) will be 40955801449 .
Regarding the last statement, do you mean changing ull to Ull ?

Yes, I meant fast_power(2, 100) in the first one. Fixed it.

I don't think you need to output modulo a specific number. The result will automatically be modulo MAX_ULL, which is 2⁶⁴-1.

No, ull is fine. I meant you need to add the suffix U to unsigned numbers greater than 2⁶³-1 to prevent warnings.

Aha! Sorry for the misunderstanding.

faheel · 2017-07-08T06:46:35Z

NumberTheory/FastPower.cpp

+int main()
+{
+    //Testing the function
+    int base,exponent;


~~int~~ ull

faheel · 2017-07-08T07:22:30Z

Test/NumberTheory/fastPower.test.cpp

+
+TEST_CASE("Base cases", "[fast_power]") {
+    REQUIRE(fast_power(2,2) == 4);
+    REQUIRE(fast_power(2,4) == 16);


Under "Base cases", check the values of the following:

fast_power(0, 0) is undefined or NaN. Comment this case here and handle it properly in the code.

fast_power(0, 1) should be 0

fast_power(1, 0) should be 1

fast_power(1, 1) should be 1

faheel · 2017-07-08T12:42:01Z

Test/NumberTheory/FastPower.test.cpp

+    REQUIRE(fast_power(1,1) == 1);
+    REQUIRE(fast_power(2,2) == 4);
+    REQUIRE(fast_power(2,2) == 4);
+    REQUIRE(fast_power(2,4) == 16);


The last 2 can be added under "Normal cases". Also the (2, 2) case has a duplicate.

ahmad307 · 2017-07-09T12:12:26Z

Requested changes are all done. Are there any other issues to be fixed?

faheel · 2017-07-11T01:58:05Z

Test/NumberTheory/FastPower.test.cpp

+
+TEST_CASE("Overflow cases", "[fast_power]") {
+    REQUIRE(fast_power(2,100) == 68719476736);
+    REQUIRE(fast_power(10,99) == 4440381706574496940U);


Both these tests are failing, and I think the reason is that at some step of the calculation, the multiplication produces the value 2⁶⁴, which for a 64-bit unsigned integer (unsigned long long) is 0, and so the final value also becomes 0 (as anything multiplied by 0 is 0).
This could be solved by taking the modulus of the values at each step, like you suggested previously, but the value of that (prime) number should be as close as possible to 2⁶⁴-1, to get exact results for a larger range of values.

I have tried returning the result modulo several prime numbers. The number 1000000000007 seems to be suitable, as a number bigger than it will result in fast_power(2,100) being greater than 2^100 mod (2^64-1).
fast_power(2,100) now returns 40955801449.
However, cases like fast_power(33,33) still cause overflow.
What do you think?

I thought about this for a while and I think what we need to do is check how many digits the result is going to take. That would be done as follows:

digits_required = floor(exponent * log10(base)) + 1

If digits_required > 19, then use modulo 1000000000007 and tell the user about this too. Otherwise, we can directly use the result.

Great idea!
but I want to note that we apply modulus 1000000000007 every time we return a value in fast_power() to prevent overflow in the function.
So we may create two functions, one that uses %1000000000007 and one that doesn't. But to prevent this, I added a third parameter to the function called "mod" where we apply %1000000000007 if digits_required > 19 or else apply %10^18 which is a number that won't affect the output result.
This is how the code is right now:

if (digits_required > 19) { cout<<endl<<fast_power(base,exponent,1000000000007)<<endl; cout<<"*The output is modulo 10^12+7"<<endl; } else { cout<<endl<<fast_power(base,exponent,1000000000000000000)<<endl; }

Shall I commit with the final changes so we can close the PR and put "FastPower" behind our backs? if that's not the most convenient thing to do right now I have no problem.

Yes, passing a third parameter is the right way instead of having 2 similar functions. But use ULLONG_MAX as the mod value in the else case instead of 10¹⁸.

10¹²+7 is not prime! So the modulo may be 0 in some cases. Use 10⁹+7 instead.

faheel · 2017-07-19T12:43:39Z

There have been lots of suggestions recently by interested contributors to add more algorithms and data structures to the project, which I have currently put on hold because I want to close issue #74 first, as it would require a complete reorganisation of files and some refactoring of code.

I previously thought that I would work on #74 once the currently active PRs are merged, but I feel that it would take much longer that way. So, if it's not too inconvenient for you, I would like to work on issue #74 as soon as possible. The remaining work on this PR (and others) can be carried on afterwards. Once the project is refactored, all you would need to do is git fetch upstream and move the code that you wrote to the new appropriate directory.

So, what do you say?

faheel

One last change and then we're good to merge!

faheel · 2017-07-20T10:48:29Z

NumberTheory/FastPower.cpp


 using namespace std;
 typedef unsigned long long ull;

 //Function that returns base raised to the exponent
-ull fast_power (ull base,ull exponent)
+ull fast_power (ull base,ull exponent,ull mod)


Give mod the default value ULLONG_MAX so that it doesn't need to be passed to the function if modulus of the result is not required.

faheel · 2017-07-20T13:05:48Z

Test/NumberTheory/FastPower.test.cpp

+    //REQUIRE(fast_power(0,0) == undefined);
+    REQUIRE(fast_power(0,1,ULLONG_MAX) == 0);
+    REQUIRE(fast_power(1,0,ULLONG_MAX) == 1);
+    REQUIRE(fast_power(1,1,ULLONG_MAX) == 1);


ULLONG_MAX should be removed from the tests too. You missed this.

Sure! Sorry for this.

faheel · 2017-07-20T13:40:07Z

Congratulations on your first PR merge! 🎉 And thanks for contributing to the project 😃

ahmad307 · 2017-07-20T14:44:44Z

Thank you! 😃 Was great working with you 😄

Added Fast Power

860bd1d

faheel requested changes Jul 6, 2017

View reviewed changes

faheel added the Number theory label Jul 6, 2017

Updated fast Power

546bc5b

I have updated the file with the required changes alongside adding sample tests.

faheel reviewed Jul 6, 2017

View reviewed changes

ahmad307 added 3 commits July 8, 2017 00:27

Edited fastPower.cpp

348ad07

Removed test cases from the source file.

Added fastPower test file

60b4356

Merge branch 'master' of https://github.com/ahmad307/Algos

e0d5338

faheel requested changes Jul 8, 2017

View reviewed changes

ahmad307 added 2 commits July 8, 2017 14:36

Issues Fixed

98fd22b

Renamed fastPower.test.cpp to FastPower.test.cpp

f61dcfd

faheel reviewed Jul 8, 2017

View reviewed changes

Duplicate removed

ea182d5

faheel requested changes Jul 11, 2017

View reviewed changes

faheel added the Algorithm label Jul 13, 2017

Updated FastPower.cpp & FastPower.test.cpp

7bee107

faheel requested changes Jul 20, 2017

View reviewed changes

Updated FastPower.cpp

e90a43e

faheel reviewed Jul 20, 2017

View reviewed changes

Updated FastPower.test.cpp

c4255a0

faheel approved these changes Jul 20, 2017

View reviewed changes

faheel merged commit 97d92b3 into ProAlgos:master Jul 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Fast Power in Number Theory #76

Added Fast Power in Number Theory #76

ahmad307 commented Jul 5, 2017 •

edited by faheel

Loading

faheel left a comment

faheel Jul 6, 2017

faheel Jul 6, 2017

faheel Jul 6, 2017

faheel Jul 6, 2017

faheel Jul 6, 2017

faheel Jul 6, 2017

ahmad307 commented Jul 6, 2017 •

edited by faheel

Loading

faheel commented Jul 6, 2017

faheel Jul 6, 2017

ahmad307 commented Jul 7, 2017

faheel left a comment

faheel Jul 8, 2017 •

edited

Loading

ahmad307 Jul 8, 2017

faheel Jul 8, 2017

ahmad307 Jul 8, 2017

faheel Jul 8, 2017

faheel Jul 8, 2017

faheel Jul 8, 2017

ahmad307 commented Jul 9, 2017

faheel Jul 11, 2017

ahmad307 Jul 15, 2017

faheel Jul 19, 2017

ahmad307 Jul 19, 2017

faheel Jul 19, 2017

faheel Jul 19, 2017 •

edited

Loading

faheel commented Jul 19, 2017

faheel left a comment

faheel Jul 20, 2017

faheel Jul 20, 2017

ahmad307 Jul 20, 2017

faheel commented Jul 20, 2017

ahmad307 commented Jul 20, 2017

Added Fast Power in Number Theory #76

Added Fast Power in Number Theory #76

Conversation

ahmad307 commented Jul 5, 2017 • edited by faheel Loading

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

ahmad307 commented Jul 6, 2017 • edited by faheel Loading

faheel commented Jul 6, 2017

Choose a reason for hiding this comment

ahmad307 commented Jul 7, 2017

faheel left a comment

Choose a reason for hiding this comment

faheel Jul 8, 2017 • 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

ahmad307 commented Jul 9, 2017

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 Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

faheel commented Jul 19, 2017

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

faheel commented Jul 20, 2017

ahmad307 commented Jul 20, 2017

ahmad307 commented Jul 5, 2017 •

edited by faheel

Loading

ahmad307 commented Jul 6, 2017 •

edited by faheel

Loading

faheel Jul 8, 2017 •

edited

Loading

faheel Jul 19, 2017 •

edited

Loading