-
-
Notifications
You must be signed in to change notification settings - Fork 482
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
rings/infinite polynomial ring fixes #37761
rings/infinite polynomial ring fixes #37761
Conversation
…t_constructor, fix coefficients
…rited method is correct)
x = self._base(x) | ||
x = self._base.coerce(x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this change to be slightly dangerous. The element construction is the conversion, but now you are requiring the input to satisfy a coercion into the base. So it might not be able to convert rational numbers into integers (e.g., 2/1
). Please add some appropriate examples showing that InfinitePolynomialRing(ZZ, 'x')
works as expected with integers realized as rational numbers and similar polynomials.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you meant as follows:
sage: R = InfinitePolynomialRing(ZZ, "a")
sage: R(GF(5)(2))
2
? (There is something odd, because the 2 above has parent ZZ
, not InfinitePolynomialRing(ZZ, "a")
.
However, the change to x = self._base.coerce(x)
is actually what resolved the bug
sage: L.<x, y> = QQ[]
sage: R.<a> = InfinitePolynomialRing(QQ)
sage: M = InfinitePolynomialRing(L, names=["a"])
sage: c = a[0]
sage: M(c)
a_0
Moreover, this is also done in MPolynomialRing_polydict.__call__
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before your changes, we have this:
sage: R = InfinitePolynomialRing(ZZ, "a")
sage: R(QQ(2)).parent()
Infinite polynomial ring in a over Integer Ring
sage: R(GF(5)(2)).parent()
Infinite polynomial ring in a over Integer Ring
The base ring can do these conversions, so the polynomial ring should also do these conversions:
sage: R.<x> = ZZ[]
sage: R(QQ(2)).parent()
Univariate Polynomial Ring in x over Integer Ring
sage: R(GF(5)(2)).parent()
Univariate Polynomial Ring in x over Integer Ring
sage: R.<x,y> = ZZ[]
sage: R(QQ(2)).parent()
Multivariate Polynomial Ring in x, y over Integer Ring
sage: R(GF(5)(2)).parent()
Multivariate Polynomial Ring in x, y over Integer Ring
However, these fail (as they should!):
sage: ZZ.coerce(GF(5)(2))
sage: ZZ.coerce(QQ(5)(2))
So this change at least completely changes how these are handled, but they should be processed here. My guess is something less straightforward with a conversion-without-coercion that cannot be done using string representations will break.
Note that the example works without your changes once the internal poly ring of has more variables than L
.
We probably need to check the input is a "compatible" infinite polynomial ring before trying to see if it belongs to the base ring. I am thinking compatible means the generator names are a subset of self
's. Likely a complete rewrite of _element_constructor_
is needed.
This might work as a short-term hack, but it is 100% a hack solution IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that a complete rewrite of _element_constructor_
is needed, but I'm a bit afraid that this is not easy. In particular, comments like "it's even more of a shame that MPolynomialRing_polydict does not work in complicated settings" don't help improving my confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be the next thing we try to do after we get the functional solver working. As such, I can accept this if we put a (code) comment saying this should instead do a conversion instead of a coercion as a # FIXME
.
Also note that I don't think this works if I want to be evil and have my base ring have variable names the same as the inf poly ring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in our recent email exchange, we've effectively found an example where this change produces undesirable results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would be good to make this example available somehow for the afterworld, I am not extremely motivated, though.
Co-authored-by: Matthias Köppe <[email protected]>
Co-authored-by: Matthias Köppe <[email protected]>
Co-authored-by: Matthias Köppe <[email protected]>
Co-authored-by: Matthias Köppe <[email protected]>
Co-authored-by: Travis Scrimshaw <[email protected]>
Co-authored-by: Travis Scrimshaw <[email protected]>
Documentation preview for this PR (built with commit 3cdc7ad; changes) is ready! 🎉 |
@tscrim, anything else you would like me to do here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some last little details. Otherwise LGTM.
Co-authored-by: Travis Scrimshaw <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Let's get this in.
Fix sagemath#37756 URL: sagemath#37761 Reported by: Martin Rubey Reviewer(s): Martin Rubey, Travis Scrimshaw
Fix #37756