Skip to content
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

avoid unnecessary divisions and calls to gcd #38924

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/sage/categories/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,11 @@ def gcd(self,other):
sage: gcd(0.0, 0.0) # needs sage.rings.real_mpfr
0.000000000000000
TESTS::
sage: QQbar(0).gcd(QQbar.zeta(3))
1
AUTHOR:
- Simon King (2011-02) -- :issue:`10771`
Expand All @@ -652,7 +657,7 @@ def gcd(self,other):
from sage.rings.integer_ring import ZZ
try:
return P(ZZ(self).gcd(ZZ(other)))
except TypeError:
except (TypeError, ValueError):
Copy link
Collaborator Author

@mantepse mantepse Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly this is wrong. Should we always raise a TypeError in _integer_? Currently we don't, for example, in QmodnZ_Element, ComplexIntervalFieldElement, ComplexBall, RealBall, LaurentPolynomial, LocalizationElement, AlgebraicNumber, AlgebraicReal, RealIntervalFieldElement, pAdicZZpXCAElement, pAdicZZpXFMElement, pAdicZZpXCRElement, pAdicCappedRelativeElement.

Or should it actually always be a ValueError?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is good to widen what we catch. Yes, we probably should be more consistent, but that isn't a reason for this to fail. Do you have an example where it was breaking before this change?

pass

if self == P.zero() and other == P.zero():
Expand Down
76 changes: 46 additions & 30 deletions src/sage/categories/unique_factorization_domains.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ def _gcd_univariate_polynomial(self, f, g):
sage: S.<T> = R[]
sage: p = (-3*x^2 - x)*T^3 - 3*x*T^2 + (x^2 - x)*T + 2*x^2 + 3*x - 2
sage: q = (-x^2 - 4*x - 5)*T^2 + (6*x^2 + x + 1)*T + 2*x^2 - x
sage: quo,rem=p.pseudo_quo_rem(q); quo,rem
sage: quo, rem = p.pseudo_quo_rem(q); quo, rem
((3*x^4 + 13*x^3 + 19*x^2 + 5*x)*T + 18*x^4 + 12*x^3 + 16*x^2 + 16*x,
(-113*x^6 - 106*x^5 - 133*x^4 - 101*x^3 - 42*x^2 - 41*x)*T
- 34*x^6 + 13*x^5 + 54*x^4 + 126*x^3 + 134*x^2 - 5*x - 50)
Expand All @@ -167,50 +167,66 @@ def _gcd_univariate_polynomial(self, f, g):
sage: g = 4*x + 2
sage: f.gcd(g).parent() is R
True

A slightly more exotic base ring::

sage: P = PolynomialRing(QQbar, 4, "x")
sage: p = sum(QQbar.zeta(i + 1) * P.gen(i) for i in range(4))
sage: ((p^4 - 1).gcd(p^3 + 1) / (p + 1)).is_unit()
True
"""
def content(X):
"""
Return the content of ``X`` up to a unit.
"""
# heuristically, polynomials tend to be monic
X_it = reversed(X.coefficients())
x = next(X_it)
if x.is_unit():
return None
for c in X_it:
x = x.gcd(c)
if x.is_unit():
return None
return x

if f.degree() < g.degree():
A,B = g, f
A, B = g, f
else:
A,B = f, g
A, B = f, g

if B.is_zero():
return A

a = b = self.zero()
for c in A.coefficients():
a = a.gcd(c)
if a.is_one():
break
for c in B.coefficients():
b = b.gcd(c)
if b.is_one():
break

d = a.gcd(b)
A = A // a
B = B // b
g = h = 1

delta = A.degree()-B.degree()
_,R = A.pseudo_quo_rem(B)
a = content(A)
if a is not None:
A //= a
b = content(B)
if b is not None:
B //= b

one = A.base_ring().one()
if a is not None and b is not None:
d = a.gcd(b)
else:
d = one
g = h = one
delta = A.degree() - B.degree()
_, R = A.pseudo_quo_rem(B)
while R.degree() > 0:
A = B
B = R // (g*h**delta)
h_delta = h**delta
B = R // (g * h_delta)
g = A.leading_coefficient()
h = h*g**delta // h**delta
h = h * g**delta // h_delta
delta = A.degree() - B.degree()
_, R = A.pseudo_quo_rem(B)

if R.is_zero():
b = self.zero()
for c in B.coefficients():
b = b.gcd(c)
if b.is_one():
break

return d*B // b

b = content(B)
if b is None:
return d * B
return d * B // b
return f.parent()(d)

class ElementMethods:
Expand Down
Loading