num-integer icon indicating copy to clipboard operation
num-integer copied to clipboard

Implement more number theory functions

Open vks opened this issue 6 years ago • 11 comments

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.

vks avatar Jul 02 '18 14:07 vks

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.

cuviper avatar Jul 12 '18 22:07 cuviper

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.

vks avatar Jul 13 '18 00:07 vks

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.

cuviper avatar Jul 13 '18 00:07 cuviper

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.

vks avatar Jul 16 '18 07:07 vks

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.

strake avatar Dec 12 '18 19:12 strake

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

carbotaniuman avatar Nov 19 '20 13:11 carbotaniuman

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

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

vks avatar Dec 01 '20 23:12 vks

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.

cuviper avatar Dec 22 '20 19:12 cuviper

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 avatar Jan 22 '21 18:01 vks

@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.

vks avatar Feb 04 '21 15:02 vks

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

cmpute avatar Apr 22 '22 18:04 cmpute