gcd of rationals is trouble

[ffc49b07-0ea9-4bb7-be61-8d0cf4baa5a0]

The following was solved along the way. We add a doctest.

The GCD of rationals is still unclear (see trac 3214), and leads to definite problems with reduce().

K.<k>= QQ[];
print(gcd(64,256))
print(gcd(K(64),K(256)))
print(gcd(64*k^2+128,64*k^3+256))
frac = (64*k^2+128)/(64*k^3+256)
frac.reduce()
print(frac)

gives

64
1
1
(64*k^2 + 128)/(64*k^3 + 256)

The last line in particular is false, according to me.

Component: basic arithmetic

Keywords: sd109

Stopgaps: todo

Author: Jonathan Kliem

Branch/Commit: bed3abb

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/8111

[burcin]

comment:1

I think the trouble here is our generic fraction field code, not how we define the gcd of rational numbers.

For efficiency, we should represent QQ(x) as Frac(ZZ[x]), and do the necessary normalisation of the denominator (it should be monic) when the user accesses it with .denominator().

[kcrisman]

comment:2

10771 is probably related/same thing.

[simon-king-jena]

comment:3

Replying to @kcrisman:

10771 is probably related/same thing.

It may be related, but my patch from #10771 does not touch the gcd for QQ['x'] (perhaps it should?). So far, the two tickets are about different issues.

[simon-king-jena]

comment:4

Replying to @simon-king-jena:

Replying to @kcrisman:

10771 is probably related/same thing.

It may be related, but my patch from #10771 does not touch the gcd for QQ['x'] (perhaps it should?). So far, the two tickets are about different issues.

PS: It seems to me that for changing gcd for univariate polynomials over the rationals, one has to dive into flint. I'll not do that, it'd be too far off topic for me. BTW, the doc string explicitly states that gcd in QQ['x'] returns the monic gcd.

[kcrisman]

comment:9

Possibly related: this discussion.

[ea1d0bf8-c27a-4548-8cb7-de0b1d02441a]

Stopgaps: todo

[kliem]

Description changed:

--- 
+++ 
@@ -1,13 +1,26 @@
+This appears to be solved by now.
+
+The output is now
+
+```
+64
+1
+1
+(k^2 + 2)/(k^3 + 4)
+```
+
+Original description (updated to python3):
+
 The GCD of rationals is still unclear (see trac 3214), and leads to definite problems with reduce(). 

K.= QQ[]; -print gcd(64,256) -print gcd(K(64),K(256)) -print gcd(64k^2+128,64k^3+256) +print(gcd(64,256)) +print(gcd(K(64),K(256))) +print(gcd(64k^2+128,64k^3+256)) frac = (64k^2+128)/(64k^3+256) frac.reduce() -print frac +print(frac)

 gives

[kliem]

Changed keywords from none to sd109

[kliem]

Author: Jonathan Kliem

[kcrisman]

Changed author from Jonathan Kliem to none

[kcrisman]

comment:12

If this is fixed, we should probably have a doctest then. Unless it wasn't an error to begin with? Or it's possible it was fixed elsewhere and doctested, which is fine too.

[kliem]

Commit: bed3abb

[kliem]

New commits:

bed3abb

add doctest for 8111

[kliem]

Description changed:

--- 
+++ 
@@ -1,15 +1,4 @@
-This appears to be solved by now.
-
-The output is now
-
-```
-64
-1
-1
-(k^2 + 2)/(k^3 + 4)
-```
-
-Original description (updated to python3):
+The following was solved along the way. We add a doctest.

 The GCD of rationals is still unclear (see trac 3214), and leads to definite problems with reduce(). 

[kliem]

Branch: public/8111

[kliem]

Author: Jonathan Kliem

[mkoeppe]

Reviewer: Matthias Koeppe

[kliem]

comment:16

Thank you.

[vbraun]

Changed branch from public/8111 to bed3abb