New pow function #10

abelmarm · 2017-12-24T17:51:03Z

pow(my_big_int, x) has been added along with a change to the name of the function abs -> big_abs to avoid conflict with cmath.

relates to #5

abelmarm · 2017-12-24T18:12:27Z

I just fall in that the pow function will have the same problem as the abs function, it could be renamed or made static.

faheel · 2017-12-24T21:14:02Z

~~You're right, there will be conflicts if we simply have pow, sqrt etc. I think we should make all the math functions static, as that's the standard way.~~

UPDATE: I just tested it after including the released file, cmath and even cstdlib. No warnings or errors.

faheel · 2017-12-24T21:15:36Z

include/functions/math.hpp

    ---
    Returns the absolute value of a BigInt.
 */

-BigInt abs(const BigInt& num) {
+const BigInt big_abs(const BigInt& num) {


Why return a const BigInt and not a BigInt?

To be honest, I was just trying things to resolve a problem.

faheel · 2017-12-24T21:16:15Z

include/functions/math.hpp

+    Return a BigInt equal to my_big_int^x
+    NOTE: x could be BigInt, long long, or string
+*/
+BigInt pow(BigInt my_big_int, const BigInt x) {


I made a mistake while mentioning the parameters for pow (sorry!). I've updated them now. It should be
(const T& base, const long long& exponent)
where the type T is BigInt, long long and std::string.

The exponent should only be an integer (long long), as I can't seem to think of situations where a BigInt would need to be raised to an exponent greater than 2⁶³-1 (it would require > 86 EiB of RAM!).

Sure, on it

abelmarm · 2017-12-25T12:04:30Z

Hmmmm. about the cmath library, maybe I goofed somewhere, but wouldn't it still be better if they were member functions, that way an user can use them without the need to additionally add the respective library and it would be easier to know about them without reading all the doc.

faheel · 2017-12-25T12:31:49Z

All the user needs to do is #include "BigInt.hpp" (the released file), irrespective of whether the math functions are member functions or not.
If they were member functions, you wouldn't be able to write:
```
BigInt big_num = pow("123", 12345);
```
This can be helpful as there's no need to first create a temporary and then call pow on it:
```
BigInt big_temp = 123;
BigInt big_num = big_temp.pow(12345);
```
Similarly for gcd and lcm.

abelmarm · 2017-12-25T15:12:47Z

I have changed the pow function to match the new requirements, but including:

#include "operators/relational.hpp"
#include "operators/unary_arithmetic.hpp"

Gives me an error when compiling.

faheel · 2017-12-25T15:29:44Z

What is the error that you get?

faheel

All non-integral parameters (including integers greater than int) should be passed as constant references (const Type&).

faheel · 2017-12-25T15:30:56Z

include/functions/math.hpp

+    Return a BigInt equal to my_big_int^x
+    NOTE: x could be BigInt, long long, or string
+*/
+BigInt pow(BigInt my_big_int, long long x) {


The parameters should be passed as constant references (const Type&).

faheel · 2017-12-25T15:33:28Z

test/functions/math_functions.test.cpp

+TEST_CASE("abs", "[conversion][string]") {
+	BigInt a("-12");
+	BigInt b("32");
+	BigInt c("-50");


This file should be test/functions/math.test.cpp. It already exists. You can add just the test cases for pow().

abelmarm · 2017-12-25T16:46:27Z

Ok, I think I found the problem, when testing the pow function I get this error:

Creating binary: bin/functions/math.test ... build/functions/math.test.o: In function `pow(BigInt const&, long long const&)':
math.test.cpp:(.text+0x1dd2): undefined reference to `BigInt::operator/(BigInt const&) const'
math.test.cpp:(.text+0x1f21): undefined reference to `BigInt::operator%(long long const&) const'
math.test.cpp:(.text+0x1f68): undefined reference to `BigInt::operator*=(BigInt const&)'
math.test.cpp:(.text+0x1f8c): undefined reference to `BigInt::operator/=(long long const&)'
math.test.cpp:(.text+0x1faa): undefined reference to `BigInt::operator*=(BigInt const&)'
math.test.cpp:(.text+0x1fc3): undefined reference to `BigInt::operator*=(BigInt const&)'
math.test.cpp:(.text+0x1ff6): undefined reference to `BigInt::operator-(long long const&) const'
math.test.cpp:(.text+0x2010): undefined reference to `BigInt::operator/(long long const&) const'
math.test.cpp:(.text+0x2063): undefined reference to `BigInt::operator*(BigInt const&) const'

Which is because I have not added the library binary_arithmetic.hpp in the math.hpp file, the problem is that when I add this library I get this other error in return when compiling:

Compiling object: build/functions/math.test.o ... In file included from include/functions/math.hpp:14:0,
                 from test/functions/math.test.cpp:7:
include/operators/binary_arithmetic.hpp: In member function ‘BigInt BigInt::operator-(const BigInt&) const’:
include/operators/binary_arithmetic.hpp:92:18: error: cannot convert ‘const BigInt’ to ‘int’ for argument ‘1’ to ‘int abs(int)’
     if (abs(*this) > abs(num)) {
                  ^
include/operators/binary_arithmetic.hpp:92:29: error: cannot convert ‘const BigInt’ to ‘int’ for argument ‘1’ to ‘int abs(int)’
     if (abs(*this) > abs(num)) {
                             ^
.
.
.

faheel · 2017-12-26T09:55:44Z

@abelmarm Have you been able to fix the issue you were facing? If not, please push your changes so that I can take a look and help.

abelmarm · 2017-12-26T11:03:39Z

I haven't, I have pushed the changes

faheel

These changes fix the problem

faheel · 2017-12-26T23:58:56Z

test/functions/math.test.cpp

@@ -6,6 +6,8 @@
 #include "functions/conversion.hpp"
 #include "functions/math.hpp"
 #include "operators/io_stream.hpp"
+#include "operators/relational.hpp"
+#include "operators/unary_arithmetic.hpp"


Add:

#include "operators/binary_arithmetic.hpp"

conversion.hpp and unary_arithmetic.hpp are not needed here.

faheel · 2017-12-27T00:00:34Z

include/functions/math.hpp


 #include "operators/relational.hpp"
 #include "operators/unary_arithmetic.hpp"
-
+#include "operators/binary_arithmetic.hpp"


Remove all operator/ includes.

faheel

Try different values in different test cases.

faheel · 2017-12-27T00:06:11Z

test/functions/math.test.cpp

@@ -6,6 +6,8 @@
 #include "functions/conversion.hpp"
 #include "functions/math.hpp"
 #include "operators/io_stream.hpp"
+#include "operators/relational.hpp"
+#include "operators/unary_arithmetic.hpp"


conversion.hpp and unary_arithmetic.hpp are not needed here.

faheel · 2017-12-27T00:08:22Z

test/functions/math.test.cpp

+
+	long long a1 = 12;
+	long long b1 = 32;
+	long long d1 = 23;


Directly pass these values to the function if they're not to be reused.

faheel · 2017-12-27T00:12:42Z

test/functions/math.test.cpp

+TEST_CASE("Pow with BigInt", "[conversion][string]") {
+	BigInt a("12");
+	BigInt b("32");
+	BigInt c("50");


Just one BigInt object is enough. You can reassign it values as required:

BigInt big_num = 12; ... big_num = 32;

Also, its better to check for values of different orders of magnitude. Eg. 2 digit, 8 digit, 20 digit, 100 digit. You can use WolframAlpha or Python to easily generate numbers of a certain size.

faheel · 2017-12-27T00:13:43Z

test/functions/math.test.cpp

+
+    REQUIRE(pow(a, b1).to_string() == "34182189187166852111368841966125056");
+    REQUIRE(pow(b, a1).to_string() == "1152921504606846976");
+    REQUIRE(pow(c, d1).to_string() == "1192092895507812500000000000000000000000");


No need of .to_string(). Simply do:

REQUIRE(pow(big_num, 12) == "34182189187166852111368841966125056");

abelmarm · 2017-12-27T09:33:14Z

Changes done, don't mind the sqrt merge I was working on that when i did what you said.

faheel · 2017-12-27T15:26:12Z

test/functions/math.test.cpp

+    a = 32;
+    REQUIRE(pow(a, (long long)12) == "1152921504606846976");
+    a = 50;
+    REQUIRE(pow(a, (long long)23) == "1192092895507812500000000000000000000000");


No need to explicitly cast to long long.

faheel · 2017-12-27T15:26:26Z

test/functions/math.test.cpp

+    REQUIRE(pow("33", (long long)27) == "99971538734896047460249499950752967950177");
+    REQUIRE(pow("21", (long long)59) == "1025506433613486607375777617584133309366191904729927960524981845743709132117581");
+    REQUIRE(pow("42", (long long)59) == "591164210212831306083214062268562882225191490961799646040575789354982868342002274605481253142528");
+    REQUIRE(pow("50", (long long)51) == "444089209850062616169452667236328125000000000000000000000000000000000000000000000000000");


No need to explicitly cast to long long.

faheel · 2017-12-27T15:31:44Z

test/functions/math.test.cpp

+    REQUIRE(pow((long long)20, (long long)40) == "10995116277760000000000000000000000000000000000000000");
+    REQUIRE(pow((long long)30, (long long)45) == "2954312706550833698643000000000000000000000000000000000000000000000");
+    REQUIRE(pow((long long)45, (long long)30) == "39479842665806602234295041487552225589752197265625");
+    REQUIRE(pow((long long)40, (long long)20) == "109951162777600000000000000000000");


No need to explicitly cast to long long.

It doesn't compile otherwise.

I think only pow(long long, long long) causes problems. Try casting just the base as long long if that's the case.

The workaround would be to write ll at the end.

Use uppercase LL though since its easier to differentiate from 11.

faheel · 2017-12-27T15:32:56Z

Once the changes are done please squash the commits into a single commit. Then it'll be good to merge 👍

abelmarm · 2017-12-27T15:42:29Z

Everything is done and committed to a final commit.

faheel · 2017-12-27T15:45:37Z

I see, but the commits haven't been squashed into a single commit. Try the following:

git rebase -i HEAD~17
Change the action frompick to squash for all commits except the first one and save.
git push -f

Resolved conflicts

abelmarm · 2017-12-27T18:25:55Z

I had a lot of problems doing that, but I think it kinda worked at last, the conflicting files are the README because you worked on it after my last fetch, but I just hard copied it, and both math files which had the problematic includes we had talked about before.

faheel · 2017-12-28T19:55:01Z

Thanks a lot for contributing @abelmarm!

faheel · 2017-12-28T22:46:07Z

I overlooked a few things (sorry about that!). Fixed them in the commits 0031c0a and 4d0b71b.

faheel requested changes Dec 24, 2017

View reviewed changes

faheel requested changes Dec 25, 2017

View reviewed changes

faheel reviewed Dec 27, 2017

View reviewed changes

faheel requested changes Dec 27, 2017

View reviewed changes

abelmarm force-pushed the master branch 3 times, most recently from 4b4348e to c3fe8b7 Compare December 27, 2017 18:15

Added pow function to /functions

015ae31

Resolved conflicts

abelmarm force-pushed the master branch from c3fe8b7 to 015ae31 Compare December 27, 2017 18:20

Merge branch 'master' into master

c519411

faheel and others added 3 commits December 28, 2017 00:35

Merge branch 'master' into master

916229b

Fix problem with some compilers.

d9023e8

Merge branch 'master' of https://github.com/ABelmarM/BigInt

d27be48

faheel merged commit 90b17c6 into faheel:master Dec 28, 2017

faheel added the functions: math Math functions label Dec 29, 2017

New pow function #10

New pow function #10

Conversation

abelmarm commented Dec 24, 2017 • edited by faheel Loading

abelmarm commented Dec 24, 2017

faheel commented Dec 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faheel Dec 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abelmarm commented Dec 25, 2017

faheel commented Dec 25, 2017 • edited Loading

abelmarm commented Dec 25, 2017 • edited Loading

faheel commented Dec 25, 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

Choose a reason for hiding this comment

abelmarm commented Dec 25, 2017

faheel commented Dec 26, 2017

abelmarm commented Dec 26, 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 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

abelmarm commented Dec 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faheel commented Dec 27, 2017

abelmarm commented Dec 27, 2017

faheel commented Dec 27, 2017 • edited Loading

abelmarm commented Dec 27, 2017 • edited Loading

faheel commented Dec 28, 2017

faheel commented Dec 28, 2017

abelmarm commented Dec 24, 2017 •

edited by faheel

Loading

faheel commented Dec 24, 2017 •

edited

Loading

faheel Dec 24, 2017 •

edited

Loading

faheel commented Dec 25, 2017 •

edited

Loading

abelmarm commented Dec 25, 2017 •

edited

Loading

faheel commented Dec 27, 2017 •

edited

Loading

abelmarm commented Dec 27, 2017 •

edited

Loading