Check that we're working over the same ring in "coefficient" - #4478
Conversation
This is already what the docs say we should do, and it can lead to strange behavior (e.g., over tower rings) if we don't check.
We need both arguments to be in the same ring, so we lift the 2nd argument to D1.
We're working over both QQ and ZZ/p -- find coefficients over QQ and sub into ZZ/p later. Also, simplify the code to use "product" while we're at it.
We now need both arguments of "coefficient" to be in the same ring, and both t and pol are already in B. There's no reason to introduce a new copy of ZZ[t].
We now need both arguments of "coefficient" to be in the same ring, matching the documentation.
We now need both arguments to be in the same ring, matching the documentation.
|
Alright, finally whack-a-moled all the packages that were using |
|
can you give an example of something that used to work and no longer does with this PR? |
Sure: i1 : R = QQ[x,y,z,w];
i2 : f = random(2, R)
9 2 1 3 2 1 7 2 4 2 8 7 2
o2 = -x + -x*y + -y + -x*z + -y*z + 7z + -x*w + -y*w + -z*w + -w
2 9 2 5 8 3 9 3 6
o2 : R
i3 : S = ZZ[x,y,z,w];
i4 : coefficient(x^2, f) -- x^2 is in S
9
o4 = -
2
o4 : QQ |
|
weird. definitely shouldn't work! |
|
Tagging authors of packages where we updated
|
| -- 2) in general promote(promote(d,B),D) != d. | ||
| promote(D,B) := (d,B) -> ( | ||
| dcoeffs := apply(E_0..E_(n-1), m -> f(coefficient(D1_(m), DtowertoD^-1 d))); | ||
| dcoeffs := apply(E_0..E_(n-1), m -> f(coefficient(D1_(m), (DtowertoD^-1 d)^D1))); |
There was a problem hiding this comment.
What is this ^D1 doing here? This seems to change the ring.
There was a problem hiding this comment.
Yeah it's lifting the 2nd argument to D1 to match the 1st argument
MichaelABurr
left a comment
MichaelABurr
left a comment
There was a problem hiding this comment.
This mostly looks good to me. There's one change that appears to change the ring, but it seems like this is what the ring should have been originally.
|
Thank you for fixing this in InvariantRing. |
|
This looks good for me, |
|
I ran a few quick tests on |
7 participants
This is already what the docs say we should do, and it can lead to strange behavior (e.g., over tower rings a la #4452) if we don't check. Also add some unit tests.
Draft for now to test the builds -- this is a slight breaking change.
AI Disclosure
Claude Code helped me diagnose the problem, but I wrote the code myself.