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

Implement more number theory functions #10

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

Conversation

vks
Copy link
Contributor

@vks vks commented Jul 2, 2018

This implements the extended Euclidean algorithm, the modular inverse and modular exponentiation.

  • This is taken from https://github.com/vks/discrete-log and uses the traits from num-traits.
  • To avoid break backwards compatibility, everything was added as a free function. However, for example powm should be part of a trait, because bigint libraries can implement it more efficiently.
  • The trait requirements for powm are not pretty, because the existing traits were not enough.

Fixes #3.

@cuviper
Copy link
Member

cuviper commented Jul 12, 2018

Sorry I haven't reviewed this -- I'm mostly pondering what the public API should look like. I think you're probably right that powm should be a trait, and maybe the others too, but I don't want to get too fine-grained about this. I'm also not fond of public fields as in GcdResult.

@vks
Copy link
Contributor Author

vks commented Jul 13, 2018

I'm a bit concerned that the trait bounds are leaking implementation details. If we change the implementation and require different traits, it can easily become a breaking change.

I'm also not fond of public fields as in GcdResult.

Why? It is similar to returning a tuple, except that the fields are named.

@cuviper
Copy link
Member

cuviper commented Jul 13, 2018

You only need the trait bounds for a generic or default impl. The alternative is to only macro-impl the primitives here, then implement bigint separately, and leave others to their own. That's not necessarily the greatest, but how many other Integer types are there anyway?

I fought with this for Roots -- I started with a default impl, but I wasn't happy with the performance I could get generically. (That's a bit of an indictment against generic num, I know...)

My basic objection to public fields is that it makes it hard to make changes to the struct. You're right that it's an improvement over a plain tuple though, which also couldn't change. The more flexible path is an opaque struct with accessors, but then you can't easily unpack values.

@vks
Copy link
Contributor Author

vks commented Jul 16, 2018

The alternative is to only macro-impl the primitives here, then implement bigint separately, and leave others to their own. That's not necessarily the greatest, but how many other Integer types are there anyway?

This would work. The other integer types mostly implement it anyway. What should the traits look like? Putting several functions into one trait makes it hard to extend, having one trait per function is probably inconvenient.

My basic objection to public fields is that it makes it hard to make changes to the struct. You're right that it's an improvement over a plain tuple though, which also couldn't change. The more flexible path is an opaque struct with accessors, but then you can't easily unpack values.

In this case, the only possible change I can think of is returning (a / gcd(a, b), b / gcd(a, b)) in the struct as well. The field names are unlikely to change, because their names don't really matter.

@strake
Copy link
Contributor

strake commented Dec 12, 2018

We could add a private field:

pub struct GcdResult<T> {
    /// Greatest common divisor.
    pub gcd: T,
    /// Coefficients such that: gcd(a, b) = c1*a + c2*b
    pub c1: T, pub c2: T,
    seal: ()
}

It would allow adding fields in future.

@carbotaniuman
Copy link

Is there any hope for this getting merged? Would love to do modular inverses on bigint.

@vks
Copy link
Contributor Author

vks commented Dec 1, 2020

I rebased on master and made GcdResult extendable as suggested by @strake.

@cuviper Should I move the added functions to a new NumberTheory trait?

@cuviper
Copy link
Member

cuviper commented Dec 22, 2020

Extended GCD was already merged in #19, so this should now build on top of that if possible.

Should I move the added functions to a new NumberTheory trait?

Can it just be added to Integer? The new methods can have additional where constraints if needed for the default impl.

@vks
Copy link
Contributor Author

vks commented Jan 22, 2021

Extended GCD was already merged in #19, so this should now build on top of that if possible.

My implementation is not equivalent: It does not use Clone, but relies on NumRef and RefNum instead. It also consumes the input, instead of taking it by reference and forcing a clone.

The implementation currently part of Integer has some possibly redundant allocations, which are unavoidable without changing the signature of the method.

I don't know which of the two signatures is preferable. The one on master with Clone, or the one in this PR with NumRef and RefNum (which would be a breaking change)? This also affects the other functions (normalize, inverse and powm), which in their current implementation (in this PR) use NumRef and RefNum to avoid allocations.

@vks
Copy link
Contributor Author

vks commented Feb 4, 2021

@cuviper Which option do you prefer?

  1. Add NumRef and RefNum to the relevant Integer methods (avoiding unnecessary allocations but requiring a breaking change).
  2. Rewrite the new methods to be compatible with the existing interface, by introducing avoidable allocations, but without breaking changes.

@cmpute
Copy link

cmpute commented Apr 22, 2022

FWIW, there's a 'num-modular` crate that have full support for modular operations.

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 more number theory functions
5 participants