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.

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

Implementation of modulus operator for BigInteger #761

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

Conversation

hdvanegasm
Copy link
Contributor

@hdvanegasm hdvanegasm commented Jan 17, 2024

Closes #717.

In this PR, I implemented the modulus operator for BigInteger. The implementation is based on "The Art Of Computer Programming" by Donald Knuth, Section 4.3, Algorithm D, with the help and guidance from https://github.com/rust-num/num-bigint/blob/master/src/biguint/division.rs.

It's possible that this implementation needs some improvements but we can consider it as a starting point. The tests for the implementation include two parts: one for which I use random generated numbers and the other for which I used fixed constant numbers to test the modulus.

@hdvanegasm hdvanegasm requested review from a team as code owners January 17, 2024 15:22
@hdvanegasm hdvanegasm requested review from z-tech, Pratyush and mmagician and removed request for a team January 17, 2024 15:22
ff/src/biginteger/mod.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
ff/src/biginteger/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@Pratyush Pratyush left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I have a couple of questions. Could you comment on why this implementation seems much longer than, the impl in crypto-bigint?

Is the algorithm you implement much faster? Or is it handling too many cases (e.g., because it was adapted from num-bigint which handles variable length?)

@hdvanegasm
Copy link
Contributor Author

hdvanegasm commented Feb 17, 2024

Hi, thanks for the comments, and sorry for the delay.

Concerning your question. Unfortunately, I haven't known about the crypto-bigint until now. Comparing the implementations, the one presented here is longer, given that I had to add some conditionals to set the inputs to the algorithm to the correct format. For example, the algorithm implemented here requires that the most significant limb of the divisor is non-zero, so I had to remove the leading zero limbs. After the computation, I had to add the missing limbs to obtain a BigInt<N>. Also, this algorithm requires right-shifting of the dividend, which needs additional conditionals in case of an overflow because those overflow bits cannot be discarded.

Comparing the performance of both algorithms, in crypto-bigint, they claim to have a function that is constant time with respect to the length of the inputs. However, looking at The Art of Computer Programming, Vol 2, Ed. 3, Section 4.3, under certain reasonable assumptions, the author claims that the running time is $O(M \cdot N)$, where $M$ is the number of limbs of quotient and $N$ is the number of limbs of the divisor, assuming that all the limbs of the BigInt<N> divisor are non-zero. Then, the number of limbs of the quotient is the difference between the number of limbs of the dividend and the divisor plus one. Hence, the running time would be bunded by $O(NA -N^2+N)$, where $A$ is the number of limbs of the dividend. But given that $A \leq N$, the running time of dividing two BigInt<N> would be $O(N)$.

Certainly, the previous analysis shows that crypto-bigint is more efficient according to their claims.

@hdvanegasm hdvanegasm requested a review from Pratyush February 21, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement modulus (%) operator between BigInteger
3 participants