Skip to content

Commit

Permalink
gh-35876: correct parent for square root of constant polynomial
Browse files Browse the repository at this point in the history
    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes #12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes #12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
Fixes #35860.

If `f` is a polynomial that is a perfect square, then `sqrt(f)` should
also be a polynomial. However, this has not been the case for
polynomials that happen to be constant: the code returned a constant in
the base ring, not a constant polynomial. The PR fixes this (and adds a
doctest for this bug).

This change fixes the serious bug that was pointed out (and diagnosed)
by @amithazi in [issue
#35860](#35860). A single-
variable Laurent polynomial is stored as a pair `(u, n)`, where `u` is
an ordinary polynomial, and `n` is an integer that represents an offset
(i.e., multiplication by a [usually negative] power of the variable). To
add two Laurent polynomials, one of them needs to be shifted so they
have the same offset:
```
f2 = right.__u << right.__n - left.__n
```
This code assumes that the parent of `right.__u` is a polynomial ring:
if the parent of `right.__u` is `ZZ`, then the shift operator `<<`
multiplies by a power of 2, instead of shifting the polynomial, so the
result is nonsense. The PR also adds a doctest for this bug.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- #12345: short description why this is a dependency
- #34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: #35876
Reported by: DaveWitteMorris
Reviewer(s): Marc Mezzarobba
  • Loading branch information
Release Manager committed Jul 8, 2023
2 parents 6b170a4 + bdfdab9 commit eef9eea
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 1 deletion.
8 changes: 8 additions & 0 deletions src/sage/rings/laurent_series_ring_element.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,14 @@ cdef class LaurentSeries(AlgebraElement):
t^-3 + t^3 + O(t^9)
ALGORITHM: Shift the unit parts to align them, then add.
TESTS:
Verify that :trac:`35860` is fixed::
sage: R.<t> = LaurentPolynomialRing(ZZ)
sage: sqrt(t^2) + t^-1
t^-1 + t
"""
cdef LaurentSeries right = <LaurentSeries>right_m
cdef long m
Expand Down
8 changes: 7 additions & 1 deletion src/sage/rings/polynomial/polynomial_element.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -1988,6 +1988,12 @@ cdef class Polynomial(CommutativePolynomial):
False
sage: R(0).is_square()
True
Make sure :trac:`35860` is fixed::
sage: S.<x> = PolynomialRing(ZZ)
sage: is_square(S(1), True)[1].parent()
Univariate Polynomial Ring in x over Integer Ring
"""
if self.is_zero():
return (True, self) if root else True
Expand All @@ -2000,7 +2006,7 @@ cdef class Polynomial(CommutativePolynomial):
u = self._parent.base_ring()(f.unit())

if all(a[1] % 2 == 0 for a in f) and u.is_square():
g = u.sqrt()
g = self._parent(u.sqrt())
for a in f:
g *= a[0] ** (a[1] // 2)
return (True, g) if root else True
Expand Down

0 comments on commit eef9eea

Please sign in to comment.