-
Notifications
You must be signed in to change notification settings - Fork 287
nmod_poly_sqrt: throw error for non-prime modulus #2537
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
base: main
Are you sure you want to change the base?
Conversation
The nmod_poly_sqrt, nmod_poly_sqrt_series, and nmod_poly_invsqrt_series functions only work correctly for prime moduli because: 1. n_sqrtmod uses Tonelli-Shanks algorithm (requires prime) 2. gr_sqrt returns GR_UNABLE for non-fields 3. Division by 2 (gr_mul_2exp_si) fails for even moduli 4. Newton iteration requires invertible elements Previously, using these functions with composite moduli like 16 would cause a crash with an uninformative GR_MUST_SUCCEED failure message. Now they throw a clear error: 'Modulus must be prime.' Fixes flintlib#2508
|
This is quite an expensive test. I think the usual flint philosophy is to state in the documentation that the modulus must be prime. If it is not prime, then "garbage in, garbage out" applies. |
Thank you. I will close that. Then I think we need to write this in document. But I also do not see this in document. |
|
Let's wait for some of the maintainers to chime in. |
|
As the primality test indeed can be relatively expensive, it would be better to try to compute the square root, and catch when this fails. If into this (warning: untested): Then I'd guess you'd also need to intercept failure of the Even more user-friendly would be to fix For the functions wrapping to throw a more informative error, e.g.: (Optionally, do some more checks when the computation actually failed to make the error message more specific.) Actually, These functions are really a bit of a mess at the moment; there's lots of things to clean up and optimize... And as you noted in #2508, it would be nice if |
|
In this case, return 0 means there is no sqrt root or we can not find a root via the algorithm, I think we should write in the document |
|
for odd composite numbers, it will return 0 when they have a sqrt root but the algorithm can not find |
|
@fredrik-johansson please review this |
|
If we do not do primality test, we can not exacly know it is a prime number or not |
Maybe we should write in the document, the modulus must be prime |
The
nmod_poly_sqrt,nmod_poly_sqrt_series, andnmod_poly_invsqrt_seriesfunctions only work correctly for prime moduli because:n_sqrtmoduses Tonelli-Shanks algorithm (requires prime)gr_sqrtreturns GR_UNABLE for non-fieldsgr_mul_2exp_si) fails for even moduliPreviously, using these functions with composite moduli like 16 would cause a crash with an uninformative
GR_MUST_SUCCEEDfailure message.Now they throw a clear error: 'Modulus must be prime.'
Fixes #2508