Fix three polynomial SIGABRT bugs #37169 and #37317 #37172
Conversation
...because attempting to run this with unhashable elements throws the same exception, which can cause confusion in higher-level algorithms.
…exist" This reverts commit 8b38164, which is from another PR.
|
Sorry for the delay :) |
|
Worth directly linking this to: #37317 |
|
Ahh I didn't see it, thanks! |
|
Waiting for CI to do it's thing before approving, but this all seems good to me now i think |
|
It seems the CI fail is unrelated to this PR. |
|
Failure is from this PR. Please do test it yourself before coming to the conclusion that "the CI is not working". |
|
Your change to the gcd to return a monic polynomial now leads to a value error for: sage: R.<x> = Zmod(4)[]
sage: f = R(2 * x)
sage: f.gcd(f)
ValueError: leading coefficient must be invertibleSo the doctest is failing. |
|
Will fix when back, sorry for that :( |
| sage: c * Zmod(2).zero() | ||
| Traceback (most recent call last): | ||
| ... | ||
| RuntimeError: Aborted |
There was a problem hiding this comment.
Is this the best error we can give the user?
There was a problem hiding this comment.
I don't know how to properly catch/handle all erroring cases in Python. I don't even understand why this is erroring right now, so it is probably best to leave this for now. Maybe a better way is for sig_on() sig_off() to return the runtime exception message or something
There was a problem hiding this comment.
Multiplication by zero should just be fine though... right? So my guess is there's some case where the flint code ensures the parents are the same and when they're not the code crashes?
|
It’s an “no inverse modulo 4” error iirc
…On 27 Feb 2024, 10:10 +0000, sagemath/sage ***@***.***>, wrote:
sage: c = 2 * z + 1 + sage: c * Zmod(2).zero() + Traceback (most recent call last): + ... + RuntimeError: Aborted Multiplication by zero should just be fine though... right? So my guess is there's some case where the flint code ensures the parents are the same and when they're not the code crashes? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were assigned.
|
|
Ahh ok, well it would be nice to try and print this a little nicer, but if we can't then that's OK |
|
@GiacomoPope When you are free, feel free to go through it again and approve if it's good :D Thanks |
| @@ -578,15 +578,18 @@ cdef inline int celement_gcd(nmod_poly_t res, nmod_poly_t a, nmod_poly_t b, unsi | |||
| sage: (G//d)*d == G | |||
| True | |||
| """ | |||
| cdef unsigned long leadcoeff, modulus, leadcoeff_res | |||
There was a problem hiding this comment.
are these variables used?
There was a problem hiding this comment.
Nope, thanks for spotting
|
LGTM -- might be worth ensuring the branch is up to date with the master |
|
Documentation preview for this PR (built with commit 4d7c349; changes) is ready! 🎉 |
Fix #37169.
Fix #37317.
Changes: adding
sig_on()andsig_off()where appropriate.