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.

Sign up for GitHub

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

Jump to bottom

fix: make interface of NCRModuloP fail-safe #2469

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

fix: make interface of NCRModuloP fail-safe #2469

wants to merge 21 commits into from

Conversation

vil02
Copy link
Member

@vil02 vil02 commented May 21, 2023

Description of Change

The original implementation of NCRModuloP, has two problems:

  • the member variable fac is one element too short,
  • the computation math::ncr_modulo_p::NCRModuloP(100, 43).ncr(10, 3, 13) returns 2, which is incorrect since10C3 = 120 and 120%13 = 3 (note different values of p).

This PR fixes above issues and does some minor refactoring, style improvements and adds missing test cases.

Checklist

  • Added description of change
  • 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.

Notes:
Updates to fail-safe API of NCRModuloP

realstealthninja
realstealthninja requested changes May 25, 2023
View reviewed changes
math/ncr_modulo_p.cpp Show resolved Hide resolved
math/ncr_modulo_p.cpp Outdated Show resolved Hide resolved
math/ncr_modulo_p.cpp Show resolved Hide resolved
math/ncr_modulo_p.cpp Outdated Show resolved Hide resolved
math/ncr_modulo_p.cpp Outdated Show resolved Hide resolved
@Panquesito7 Panquesito7 added the bugfix Correction to existing algorithms label May 30, 2023
Panquesito7
Panquesito7 reviewed May 30, 2023
View reviewed changes
math/ncr_modulo_p.cpp Outdated Show resolved Hide resolved
math/ncr_modulo_p.cpp Outdated Show resolved Hide resolved
@vil02 vil02 requested a review from Panquesito7 May 30, 2023 21:37
Panquesito7
Panquesito7 previously approved these changes May 31, 2023
View reviewed changes
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks! 🚀

@Panquesito7 Panquesito7 added the Improvement improvement in previously written codes label May 31, 2023
realstealthninja
realstealthninja requested changes Jun 16, 2023
View reviewed changes
math/ncr_modulo_p.cpp Show resolved Hide resolved
math/ncr_modulo_p.cpp Show resolved Hide resolved
@realstealthninja
Copy link
Collaborator

Please do not resolve unless you have instructions to do so or the changes are sufficiently discussed

@vil02 vil02 requested a review from Panquesito7 June 23, 2023 21:25
@vil02
Copy link
Member Author

vil02 commented Jul 13, 2023

@Panquesito7 could you please have a final look?

Panquesito7
Panquesito7 reviewed Jul 19, 2023
View reviewed changes
math/ncr_modulo_p.cpp Outdated Show resolved Hide resolved
math/ncr_modulo_p.cpp Show resolved Hide resolved
@vil02 vil02 requested a review from Panquesito7 August 8, 2023 20:34
@vil02
Copy link
Member Author

vil02 commented Sep 8, 2023

@Panquesito7, @realstealthninja could you please have a final look?

realstealthninja
realstealthninja approved these changes Sep 9, 2023
View reviewed changes
Copy link
Collaborator

@realstealthninja realstealthninja left a comment

Choose a reason for hiding this comment

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

Looks great to me ❤️

@github-actions GitHub Actions
Copy link
Contributor

This pull request has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Author has not responded to the comments for over 2 weeks label Dec 27, 2023
@vil02
Copy link
Member Author

vil02 commented Dec 27, 2023

@Panquesito7, @realstealthninja please dont-close.

@github-actions github-actions bot removed the stale Author has not responded to the comments for over 2 weeks label Dec 28, 2023
vil02 and others added 21 commits February 12, 2024 22:23
@vil02
fix: set proper size of fac
d28c25c
@vil02
style: use std::size_t as a type of loop counter
5864c5a
@vil02
style: use uint64_t as a type of loop counter
81b3e7b
@vil02
fix: remove p from the argument list of NCRModuloP::ncr
21b2771
@vil02
refactor: add utils namespace
69f48b9
@vil02
refactor: use references in gcdExtended
e3b71c5
@vil02
refactor: add NCRModuloP::computeFactorialsMod
4672baa
@vil02
style: make NCRModuloP::ncr const
d712dc8
@vil02
test: reorganize tests
a13dff8
@vil02
test: add missing test cases
af191b5
@vil02
refactor: simplify logic
d29c96f
@vil02
style: make example object const
334d91e
@vil02
style: use auto
478edd5
@vil02
style: use int64_t to avoid narrowing conversions
80b69db
@vil02
docs: update explanation why to import iostream
63e9d5b
@vil02
docs: remove p from docstr of NCRModuloP::ncr
e67ec3e
@vil02
docs: udpate doc-strs and add example()
94a475c
@vil02 @Panquesito7
Apply suggestions from code review
6bf8ccf 
Co-authored-by: David Leal <[email protected]>
@vil02
dosc: add missing docs
c4d1f34
@vil02 @Panquesito7
feat: display message when all tests pass
857e53d 
Co-authored-by: David Leal <[email protected]>
@vil02 @Panquesito7
style: initialize NCRModuloP::p with 0
d44c270 
Co-authored-by: David Leal <[email protected]>
@realstealthninja realstealthninja added the dont-close This issue/pull request shouldn't be closed label Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madhavgaba madhavgaba madhavgaba approved these changes

@realstealthninja realstealthninja realstealthninja approved these changes

@Panquesito7 Panquesito7 Awaiting requested review from Panquesito7 Panquesito7 is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
bugfix Correction to existing algorithms dont-close This issue/pull request shouldn't be closed Improvement improvement in previously written codes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vil02 @realstealthninja @madhavgaba @Panquesito7