feat: Add ncr mod p code #1325

KaustubhDamania · 2020-10-18T11:03:47Z

Description of Change

Add code for calculating nCr modulo p.
Solves #1323

Checklist

Added description of change
Added file name matches File name guidelines
Added tests and example, test must pass
Added documentation so that the program is self-explanatory and educational - Doxygen guidelines
Relevant documentation/comments is changed or added
PR title follows semantic commit guidelines
Search previous suggestions before making a new one, as yours may be a duplicate.
I acknowledge that all my contributions will be made under the project's license.

lgtm-com · 2020-10-18T11:32:02Z

This pull request introduces 1 alert when merging ec3899b into 79b98cc - view on LGTM.com

new alerts:

1 for Short global name

lgtm-com · 2020-10-18T20:24:45Z

This pull request introduces 1 alert when merging c2f31f8 into 981f781 - view on LGTM.com

new alerts:

1 for Short global name

lgtm-com · 2020-10-18T20:37:24Z

This pull request introduces 1 alert when merging bd98630 into 981f781 - view on LGTM.com

new alerts:

1 for Short global name

KaustubhDamania · 2020-10-18T20:39:52Z

@Panquesito7 I see you have added "Proper Documentation Required". May I know what documentation do I need to add? Apart from that I have fixed the automation test errors.

math/ncr_modulo_p.cpp

Co-authored-by: David Leal <halfpacho@gmail.com>

lgtm-com · 2020-10-19T06:01:01Z

This pull request introduces 1 alert when merging e44b730 into 981f781 - view on LGTM.com

new alerts:

1 for Short global name

Panquesito7

Please enable GitHub Actions in your repository of this fork in this link: https://github.com/KaustubhDamania/C-Plus-Plus/actions

Panquesito7 · 2020-10-20T19:17:34Z

math/ncr_modulo_p.cpp

+#include <iostream>
+#include <vector>
+
+/** Finds the value of x, y such that a*x + b*y = gcd(a,b)


Suggested change

/** Finds the value of x, y such that a*x + b*y = gcd(a,b)

/**

* @namespace math

* @brief Mathematical algorithms

*/

namespace math {

/** Finds the value of x, y such that a*x + b*y = gcd(a,b)

Panquesito7 · 2020-10-20T19:18:59Z

math/ncr_modulo_p.cpp

+    }
+}
+
+std::vector<int64_t> fac;


Avoid using global variables, they may cause overflows and other issues.
Pass them as local function parameters, or as class private members.

what if I keep that variable inside namespace?
And later access them as math::fac?

Easiest thing you can do is to pass them as local function parameters.
Or even better, add them as (public) class members, and call the variable directly to the function.

Yes, I refactored the code so that the factorial vector and the remaining functions are inside a class

Panquesito7 · 2020-10-20T19:19:46Z

math/ncr_modulo_p.cpp

+    }
+
+    // (52323 C 26161) % (1e9 + 7) = 224944353
+    assert(ncr(52323, 26161, p) == 224944353);


Please add the assert checks in a separate test function. 🙂

KaustubhDamania · 2020-10-22T16:16:15Z

@Panquesito7 I have made the required changes, can you review this please?

Panquesito7 · 2020-10-22T16:39:22Z

math/ncr_modulo_p.cpp

+#include <cassert>
+#include <iostream>
+#include <vector>


Suggested change

#include <cassert>

#include <iostream>

#include <vector>

#include <cassert> /// for assert

#include <iostream> /// for io operations

#include <vector> /// for std::vector

Panquesito7 · 2020-10-22T16:39:40Z

math/ncr_modulo_p.cpp

+#include <iostream>
+#include <vector>
+
+/** Class which contains all methods required for calculating nCr mod p


Suggested change

/** Class which contains all methods required for calculating nCr mod p

/**

* @namespace math

* @brief Mathematical algorithms

*/

namespace math {

/**

* @brief Class which contains all methods required for calculating nCr mod p

Panquesito7 · 2020-10-22T16:39:44Z

math/ncr_modulo_p.cpp

+#include <vector>
+
+/** Class which contains all methods required for calculating nCr mod p
+ *


Suggested change

*

Panquesito7 · 2020-10-22T16:41:17Z

math/ncr_modulo_p.cpp

+        }
+        return (fac[n] * denominator) % p;
+    }
+};


Suggested change

};

};

} // namespace math

Panquesito7 · 2020-10-22T16:41:29Z

math/ncr_modulo_p.cpp

+    for (int i = 0; i <= 7; i++) {
+        std::cout << 6 << "C" << i << " = " << ncrObj.ncr(6, i, p) << "\n";
+    }
+    tests(ncrObj);


Suggested change

tests(ncrObj);

tests(ncrObj); // execute the tests

Panquesito7 · 2020-10-22T16:46:11Z

math/ncr_modulo_p.cpp

+    }
+};
+
+void tests(NCRModuloP ncrObj) {


Add a brief description of the ncrObj parameter here (in the code).

Suggested change

void tests(NCRModuloP ncrObj) {

/**

* @brief Test implementations

* @param ncrObj description

* @returns void

*/

void tests(NCRModuloP ncrObj) {

Panquesito7 · 2020-10-22T16:46:35Z

math/ncr_modulo_p.cpp

+ */
+class NCRModuloP {
+ private:
+    std::vector<int64_t> fac;


Please use uint64_t instead (same in other parts of the code).

@Panquesito7 I have replaced int64_t with uint64_t in all parts except the ncr and modInverse function because those functions can return a negative value in case the modular inverse doesn't exist. Also, the variables x,y in gcdExtended should be int64_t because they can be negative as well.

Panquesito7 · 2020-10-22T16:47:10Z

math/ncr_modulo_p.cpp

+/**
+ * @file
+ * @brief This program aims at calculating nCr modulo p
+ *


Suggested change

*

*

* @author [Kaustubh Damania](https://github.com/KaustubhDamania)

Panquesito7 · 2020-10-22T16:47:20Z

math/ncr_modulo_p.cpp

@@ -0,0 +1,122 @@
+/**
+ * @file
+ * @brief This program aims at calculating nCr modulo p


Please add a detailed description using @details.

Panquesito7

LGTM, good work! 😄 👍

KaustubhDamania · 2020-11-05T13:21:13Z

@ayaankhan98 can you review this? or @Panquesito7 can you merge this? 😅

Panquesito7 · 2020-11-05T19:16:01Z

math/ncr_modulo_p.cpp

+    std::vector<uint64_t> fac;
+    uint64_t p;


Suggested change

std::vector<uint64_t> fac;

uint64_t p;

std::vector<uint64_t> fac{};

uint64_t p = 0;

Panquesito7 · 2020-11-05T19:16:29Z

math/ncr_modulo_p.cpp

+     *  and stores them in vector 'fac'
+     *  @params[in] the numbers 'size', 'mod'
+     */
+    NCRModuloP(uint64_t size, uint64_t mod) {


Suggested change

NCRModuloP(uint64_t size, uint64_t mod) {

NCRModuloP(const uint64_t& size, const uint64_t& mod) {

Panquesito7 · 2020-11-05T19:33:24Z

math/ncr_modulo_p.cpp

+     * equation
+     * @returns the gcd of a and b
+     */
+    uint64_t gcdExtended(uint64_t a, uint64_t b, int64_t *x, int64_t *y) {


Suggested change

uint64_t gcdExtended(uint64_t a, uint64_t b, int64_t *x, int64_t *y) {

uint64_t gcdExtended(const uint64_t& a, const uint64_t& b, int64_t *x, int64_t *y) {

Panquesito7 · 2020-11-05T19:34:15Z

math/ncr_modulo_p.cpp

+     * @params[in] the numbers 'a' and 'm' from above equation
+     * @returns the modular inverse of a
+     */
+    int64_t modInverse(uint64_t a, uint64_t m) {


Suggested change

int64_t modInverse(uint64_t a, uint64_t m) {

int64_t modInverse(const uint64_t& a, const uint64_t& m) {

Panquesito7 · 2020-11-05T19:41:21Z

math/ncr_modulo_p.cpp

+     * @params[in] the numbers 'n', 'r' and 'p'
+     * @returns the value nCr % p
+     */
+    int64_t ncr(uint64_t n, uint64_t r, uint64_t p) {


Suggested change

int64_t ncr(uint64_t n, uint64_t r, uint64_t p) {

int64_t ncr(const uint64_t& n, const uint64_t& r, const uint64_t& p) {

Panquesito7 · 2020-11-05T19:42:07Z

math/ncr_modulo_p.cpp

+        std::cout << 6 << "C" << i << " = " << ncrObj.ncr(6, i, p) << "\n";
+    }
+    tests(ncrObj);  // execute the tests
+    std::cout << "Assertions passed\n";


Suggested change

std::cout << "Assertions passed\n";

std::cout << "Assertions passed\n";

return 0;

Panquesito7 · 2020-11-05T19:42:58Z

math/ncr_modulo_p.cpp

@@ -0,0 +1,138 @@
+/**
+ * @file
+ * @brief This program aims at calculating nCr modulo p


Provide a Wikipedia link in Markdown format (if available/possible).
If there's no Wikipedia link, add another web source for algorithm explanation. 🙂

Panquesito7 · 2020-11-05T19:44:25Z

math/ncr_modulo_p.cpp

+ * @namespace math
+ * @brief Mathematical algorithms
+ */
+namespace math {


Suggested change

namespace math {

namespace math {

/**

* @namespace ncr_modulo_p

* @brief Functions for nCr modulo p implementation.

*/

namespace ncr_modulo_p {

Panquesito7 · 2020-11-05T19:44:41Z

math/ncr_modulo_p.cpp

+        return (fac[n] * denominator) % p;
+    }
+};
+}  // namespace math


Suggested change

} // namespace math

} // namespace ncr_modulo_p

} // namespace math

Panquesito7 · 2020-11-05T19:45:19Z

math/ncr_modulo_p.cpp

+    // populate the fac array
+    const uint64_t size = 1e6 + 1;
+    const uint64_t p = 1e9 + 7;
+    math::NCRModuloP ncrObj = math::NCRModuloP(size, p);


Suggested change

math::NCRModuloP ncrObj = math::NCRModuloP(size, p);

math::ncr_modulo_p::NCRModuloP ncrObj = math::ncr_modulo_p::NCRModuloP(size, p);

Panquesito7 · 2020-11-13T19:22:01Z

math/ncr_modulo_p.cpp

+    std::vector<uint64_t> fac{};
+    uint64_t p = 0;


Add a brief description of what the variable does.

Suggested change

std::vector<uint64_t> fac{};

uint64_t p = 0;

std::vector<uint64_t> fac{}; ///< brief description

uint64_t p = 0; ///< brief description

@Panquesito7 Done! 👍

KaustubhDamania · 2020-11-14T05:59:22Z

@Panquesito7 can you review this? 😅 I have made the necessary changes, hope it gets merged soon!

math/ncr_modulo_p.cpp

Co-authored-by: David Leal <halfpacho@gmail.com>

math/ncr_modulo_p.cpp

Co-authored-by: David Leal <halfpacho@gmail.com>

Panquesito7

Awesome work! Thanks for your contribution! 🎉 👍

Panquesito7 · 2020-11-15T20:25:07Z

@ayaankhan98 @kvedala please review this PR. Thanks. 🙂

KaustubhDamania · 2020-11-20T05:45:38Z

@ayaankhan98 @kvedala can you review this please? 😅

ayaankhan98

@KaustubhDamania please resolve the merge conflict

KaustubhDamania · 2020-11-21T14:17:44Z

@KaustubhDamania please resolve the merge conflict

@ayaankhan98 done! 👍 can you review this?

ayaankhan98

LGTM!

KaustubhDamania · 2020-11-23T09:08:42Z

@ayaankhan98 @Panquesito7 Thanks for reviewing & merging my PR, could you add the hacktoberfest-accepted label to it?

Panquesito7 · 2020-11-23T16:55:54Z

could you add the hacktoberfest-accepted label

Done. 🙂

This comment has been minimized.

Sign in to view

Panquesito7 added automated tests are failing Do not merge until tests pass enhancement New feature or request Proper Documentation Required requested to write the documentation properly labels Oct 18, 2020

KaustubhDamania force-pushed the fix-1323 branch from ec3899b to c2f31f8 Compare October 18, 2020 20:13

feat: Add ncr mod p code (TheAlgorithms#1323)

bd98630

KaustubhDamania force-pushed the fix-1323 branch from c2f31f8 to bd98630 Compare October 18, 2020 20:27

Panquesito7 reviewed Oct 18, 2020

View reviewed changes

math/ncr_modulo_p.cpp Show resolved Hide resolved

Panquesito7 removed the Proper Documentation Required requested to write the documentation properly label Oct 18, 2020

Update math/ncr_modulo_p.cpp

e44b730

Co-authored-by: David Leal <halfpacho@gmail.com>

KaustubhDamania requested a review from Panquesito7 October 19, 2020 10:00

KaustubhDamania changed the title ~~feat: Add ncr mod p code (#1323)~~ feat: Add ncr mod p code Oct 19, 2020

Panquesito7 removed the automated tests are failing Do not merge until tests pass label Oct 20, 2020

Panquesito7 requested changes Oct 20, 2020

View reviewed changes

Panquesito7 added the requested changes changes required label Oct 20, 2020

KaustubhDamania and others added 3 commits October 22, 2020 12:06

Added all functions inside a class + added more asserts

f6df24a

updating DIRECTORY.md

51a1fe6

clang-format and clang-tidy fixes for f6df24a

2ba05ed

Panquesito7 requested changes Oct 22, 2020

View reviewed changes

Replace int64_t to uint64_t + add namespace + detailed documentation

e09a057

KaustubhDamania force-pushed the fix-1323 branch from d5a278f to e09a057 Compare October 22, 2020 20:00

clang-format and clang-tidy fixes for e09a057

8e9803d

Panquesito7 approved these changes Oct 22, 2020

View reviewed changes

Panquesito7 removed the requested changes changes required label Oct 22, 2020

Panquesito7 requested changes Nov 5, 2020

View reviewed changes

KaustubhDamania and others added 2 commits November 13, 2020 23:00

Add extra namespace + add const& in function arguments

8111f88

clang-format and clang-tidy fixes for 8111f88

ba954d4

Panquesito7 reviewed Nov 13, 2020

View reviewed changes

KaustubhDamania and others added 2 commits November 14, 2020 11:24

Update ncr_modulo_p.cpp

2ad2f72

clang-format and clang-tidy fixes for 2ad2f72

f8c9934

Panquesito7 reviewed Nov 15, 2020

View reviewed changes

math/ncr_modulo_p.cpp Outdated Show resolved Hide resolved

Update math/ncr_modulo_p.cpp

350508f

Co-authored-by: David Leal <halfpacho@gmail.com>

Panquesito7 reviewed Nov 15, 2020

View reviewed changes

math/ncr_modulo_p.cpp Outdated Show resolved Hide resolved

math/ncr_modulo_p.cpp Outdated Show resolved Hide resolved

KaustubhDamania and others added 2 commits November 16, 2020 01:38

Update math/ncr_modulo_p.cpp

4741898

Co-authored-by: David Leal <halfpacho@gmail.com>

Update math/ncr_modulo_p.cpp

5b69ba5

Co-authored-by: David Leal <halfpacho@gmail.com>

github-actions bot force-pushed the fix-1323 branch from 5b69ba5 to 70bc17b Compare November 15, 2020 20:09

clang-format and clang-tidy fixes for 5b69ba5

8b747e2

github-actions bot force-pushed the fix-1323 branch from 70bc17b to 8b747e2 Compare November 15, 2020 20:09

Panquesito7 approved these changes Nov 15, 2020

View reviewed changes

ayaankhan98 reviewed Nov 20, 2020

View reviewed changes

KaustubhDamania and others added 3 commits November 20, 2020 14:16

Merge branch 'master' into fix-1323

a8401d4

updating DIRECTORY.md

7f33ea8

clang-format and clang-tidy fixes for a8401d4

db28362

KaustubhDamania requested a review from ayaankhan98 November 20, 2020 12:22

ayaankhan98 approved these changes Nov 22, 2020

View reviewed changes

ayaankhan98 merged commit 67e26cf into TheAlgorithms:master Nov 22, 2020

Panquesito7 added the hacktoberfest-accepted Accepted to be counted towards Hacktoberfest label Nov 23, 2020

-void tests(NCRModuloP ncrObj) {
+/**
+ * @brief Test implementations
+ * @param ncrObj description
+ * @returns void
+ */
+void tests(NCRModuloP ncrObj) {

	*
	*
	* @author [Kaustubh Damania](https://github.com/KaustubhDamania)

	NCRModuloP(uint64_t size, uint64_t mod) {
	NCRModuloP(const uint64_t& size, const uint64_t& mod) {

	uint64_t gcdExtended(uint64_t a, uint64_t b, int64_t x, int64_t y) {
	uint64_t gcdExtended(const uint64_t& a, const uint64_t& b, int64_t x, int64_t y) {

	int64_t modInverse(uint64_t a, uint64_t m) {
	int64_t modInverse(const uint64_t& a, const uint64_t& m) {

	int64_t ncr(uint64_t n, uint64_t r, uint64_t p) {
	int64_t ncr(const uint64_t& n, const uint64_t& r, const uint64_t& p) {

-namespace math {
+namespace math {
+/**
+ * @namespace ncr_modulo_p
+ * @brief Functions for nCr modulo p implementation.
+ */
+namespace ncr_modulo_p {

	} // namespace math
	} // namespace ncr_modulo_p
	} // namespace math

	math::NCRModuloP ncrObj = math::NCRModuloP(size, p);
	math::ncr_modulo_p::NCRModuloP ncrObj = math::ncr_modulo_p::NCRModuloP(size, p);

feat: Add ncr mod p code #1325

feat: Add ncr mod p code #1325

Conversation

KaustubhDamania commented Oct 18, 2020 • edited by gitpod-io bot

Description of Change

Checklist

This comment has been minimized.

lgtm-com bot commented Oct 18, 2020

lgtm-com bot commented Oct 18, 2020

lgtm-com bot commented Oct 18, 2020

KaustubhDamania commented Oct 18, 2020 • edited

lgtm-com bot commented Oct 19, 2020

Panquesito7 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

KaustubhDamania commented Oct 22, 2020

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

Panquesito7 left a comment

Choose a reason for hiding this comment

KaustubhDamania commented Nov 5, 2020

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

KaustubhDamania Nov 15, 2020 • edited

Choose a reason for hiding this comment

KaustubhDamania commented Nov 14, 2020

Panquesito7 left a comment

Choose a reason for hiding this comment

Panquesito7 commented Nov 15, 2020

KaustubhDamania commented Nov 20, 2020

ayaankhan98 left a comment

Choose a reason for hiding this comment

KaustubhDamania commented Nov 21, 2020

ayaankhan98 left a comment

Choose a reason for hiding this comment

KaustubhDamania commented Nov 23, 2020 • edited

Panquesito7 commented Nov 23, 2020

KaustubhDamania commented Oct 18, 2020 •

edited by gitpod-io bot

KaustubhDamania commented Oct 18, 2020 •

edited

KaustubhDamania Nov 15, 2020 •

edited

KaustubhDamania commented Nov 23, 2020 •

edited