nim-lang / bigints

BigInts for Nim
MIT License
124 stars 32 forks source link

Add modular exponentiation and modular inverse, closes #45 #65

Closed dlesnoff closed 2 years ago

dlesnoff commented 2 years ago

Add powmod with all the comments made on the previous pull request.

dlesnoff commented 2 years ago

I finally made a rebase as the changes seemed too minor for a commit. I hope this will not break something, as I had to force push into my branch.

dlesnoff commented 2 years ago

I have added modular inverse for negative exponents in modular exponentiation. I fear that modular inverse is too slow as for now. It might be improved with a faster multiplication. Fixes #45.

dlesnoff commented 2 years ago

I have changed the modular exponentiation so that it uses an if..elif..else construct rather than only ifs. I optimized a bit by using the isZero function rather than a modulus == 0. I added some tests for two other edge cases (0^0 mod n == 1 and 0^1 mod n == 0). I have tried to rebase on latest commit but I did this uncorrectly (I merged and rebased, while I should have done a simple rebase).

dlesnoff commented 2 years ago

Fix PR's history. The 5 different commits might be too much, wich ones should I keep ?

konsumlamm commented 2 years ago

Fix PR's history. The 5 different commits might be too much, wich ones should I keep ?

Don't worry, PRs get squashed anyway, it doesn't really matter how many commits your PR has.

dlesnoff commented 2 years ago

I believe there are some cases where invmod returns negative integers depending on the sign of the input. This needs more testing.

dlesnoff commented 2 years ago

Tried to rebase, then merge with current master branch. History is a mess. Please squash my commits before merging.

dlesnoff commented 2 years ago

I removed an example that used a custom modular exponentiation similar to my powmod function. I could have replaced with it with just one call to the function, it would not have been interesting.

konsumlamm commented 2 years ago

@dlesnoff can you rebase this, to resolve the conflicts? @narimiran can we merge this soon, to avoid more conflicts? Unless you have any objections of course.

dlesnoff commented 2 years ago

merged the changes to resolve the conflicts.