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

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

Merged
merged 32 commits into from
Jan 22, 2022

Conversation

dlesnoff
Copy link
Contributor

@dlesnoff dlesnoff commented Jan 5, 2022

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

src/bigints.nim Outdated Show resolved Hide resolved
src/bigints.nim Outdated Show resolved Hide resolved
@dlesnoff
Copy link
Contributor Author

dlesnoff commented Jan 5, 2022

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
Copy link
Contributor Author

dlesnoff commented Jan 6, 2022

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.

src/bigints.nim Outdated
if modulus == 1:
return zero
if exponent < 0:
if base == zero:
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have base == zero return zero also for positive exponent? (and maybe we also check for exponent == 0 before and return 1 in that case)?
code would then be:

  if exponent == zero:
    return one
  if base == zero:
    return zero
  if exponent < 0:
    let baseInv = invmod(base, modulus)
    return powmod(baseInv, -exponent, modulus)

also: not sure if all those ifs would be better with an if..elif..else (even if they raise or return)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your propositiion removes a lot of unuseful computation for zero base and large positive exponent indeed.
I have not written it this way as zero does not appear as a specific case of the entire algorithm. The square and multiply algorithm would work with 0 as base too.
It is an edge case for modular inverse though and 0 has strictly speaking no modular inverse. As @konsumlamm has proposed to me to remove an if clause in previous PR, I think it is better to keep only edge cases in the first ifs (it improves readability).
Concerning multiple ifs rather than if .. elif .. else clause, I personnaly prefer to avoid else clause as it adds unuseful indentation for the rest of the algorithm. Maybe the compiler is able to do more compilation's optimization with if .. elif .. else clause. I do not think it improves a lot.

Copy link
Contributor

@konsumlamm konsumlamm left a comment

Choose a reason for hiding this comment

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

I'm not fully done with reviewing, will finish that later (hopefully today).

src/bigints.nim Outdated Show resolved Hide resolved
src/bigints.nim Outdated Show resolved Hide resolved
src/bigints.nim Outdated Show resolved Hide resolved
@dlesnoff
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@konsumlamm
Copy link
Contributor

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.

Copy link
Contributor

@konsumlamm konsumlamm left a comment

Choose a reason for hiding this comment

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

Now I'm done with reviewing. It took a little longer than expected.

src/bigints.nim Outdated Show resolved Hide resolved
src/bigints.nim Show resolved Hide resolved
src/bigints.nim Outdated Show resolved Hide resolved
src/bigints.nim Outdated Show resolved Hide resolved
src/bigints.nim Outdated Show resolved Hide resolved
dlesnoff and others added 5 commits January 10, 2022 23:49
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
@dlesnoff
Copy link
Contributor Author

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

src/bigints.nim Outdated Show resolved Hide resolved
src/bigints.nim Outdated Show resolved Hide resolved
src/bigints.nim Outdated Show resolved Hide resolved
src/bigints.nim Outdated Show resolved Hide resolved
src/bigints.nim Outdated Show resolved Hide resolved
I see no opposition to these changes.

Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
src/bigints.nim Outdated Show resolved Hide resolved
@dlesnoff dlesnoff changed the title Add modular exponentiation Add modular exponentiation and modular inverse Jan 13, 2022
@dlesnoff
Copy link
Contributor Author

dlesnoff commented Jan 16, 2022

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

@dlesnoff dlesnoff changed the title Add modular exponentiation and modular inverse Add modular exponentiation and modular inverse, closes #45 Jan 16, 2022
@dlesnoff
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor Author

merged the changes to resolve the conflicts.

@narimiran narimiran merged commit 658733d into nim-lang:master Jan 22, 2022
@dlesnoff dlesnoff deleted the devel branch January 23, 2022 16:00
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.

None yet

4 participants