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

BigUint extended_gcd panics on underflow #253

Open
JeremyRubin opened this issue Sep 8, 2022 · 7 comments
Open

BigUint extended_gcd panics on underflow #253

JeremyRubin opened this issue Sep 8, 2022 · 7 comments

Comments

@JeremyRubin
Copy link

repro: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b18dc13a3c981c6351d3cdd6e24a8715

One quick fix for this would be to override the trait default, cast to signed, and then make sure the coefficients are the positive form after (e.g., by taking the mod), if it is difficult to make the generalized algorithm work.

@cuviper
Copy link
Member

cuviper commented Sep 9, 2022

We really should have just required Signed for that method, as we did for extended_gcd_lcm, but that will take a breaking change in num-integer. There are few edge cases where everything can be unsigned -- I think only when one input is a multiple of the other, then you get 0 and 1 coefficients.

cast to signed, and then make sure the coefficients are the positive form after (e.g., by taking the mod)

Using what modulus?

@JeremyRubin
Copy link
Author

Using what modulus?

quick fix retracted :p

@matthiasgeihs
Copy link

Any news on this?

It is still an issue, see another playground example.

@cuviper
Copy link
Member

cuviper commented Apr 25, 2023

It's a bad API to have allowed BigUint at all -- please use BigInt here.

@matthiasgeihs
Copy link

Makes sense. However, I would prefer not to have to convert between BigUint and BigInt back and forth.

I simply want to compute the modular inverse of a BigUint, modulo a BigUint. Is there a better way to do it than convert to BigInt, call extended_gcd, ensure that the outcome is not negative, and convert back? It's a lot of code for a simple, common operation...

@cuviper
Copy link
Member

cuviper commented Apr 26, 2023

You could wrap BigUint in a type that does modular Sub, so it never goes negative. The conversions are not so bad though - From<BigUint> for BigInt is very cheap, and going the other way can use BigInt::into_parts, then you can apply modular negation for Sign::Minus. I don't see how you expect these non-modular types to do that for you...

@matthiasgeihs
Copy link

matthiasgeihs commented Apr 27, 2023

OK, sounds like a decent workaround. Thanks!

(With regards to the conversions: My concern there wasn't performance, but rather code readability.)

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

No branches or pull requests

3 participants