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

Conversion of rationals into the fraction field of integer polynomials #7958

Closed
sagetrac-spancratz mannequin opened this issue Jan 16, 2010 · 12 comments
Closed

Conversion of rationals into the fraction field of integer polynomials #7958

sagetrac-spancratz mannequin opened this issue Jan 16, 2010 · 12 comments

Comments

@sagetrac-spancratz
Copy link
Mannequin

sagetrac-spancratz mannequin commented Jan 16, 2010

sage: F = Frac(PolynomialRing(ZZ, 't'))
sage: F(1/2)
...
TypeError: no conversion of this rational to integer

Component: coercion

Author: Sebastian Pancratz, Mike Hansen

Reviewer: Mike Hansen, Sebastian Pancratz

Merged: sage-4.3.2.alpha0

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

@sagetrac-spancratz sagetrac-spancratz mannequin added this to the sage-4.3.2 milestone Jan 16, 2010
@sagetrac-spancratz sagetrac-spancratz mannequin assigned aghitza and robertwb and unassigned aghitza Jan 16, 2010
@sagetrac-spancratz
Copy link
Mannequin Author

sagetrac-spancratz mannequin commented Jan 16, 2010

Attachment: trac7958.patch.gz

@wjp
Copy link
Mannequin

wjp mannequin commented Jan 17, 2010

comment:3

Your fix does work great for QQ, but this is actually a more general issue than just QQ:

sage: _.<x> = ZZ[]
sage: K.<a> = NumberField(x^2-2)
sage: R.<b> = K.ring_of_integers()
sage: S.<y> = R[]
sage: F = FractionField(S)
sage: F(1)/F(a)
1/a
sage: F(1/a)
*boom*

And a minor issue: I think the comment about QQ should be a code comment rather than in the doctest, since it might now confuse users (who might think they need to handle QQ specially themselves).

@wjp wjp mannequin added s: needs work and removed s: needs review labels Jan 17, 2010
@sagetrac-spancratz
Copy link
Mannequin Author

sagetrac-spancratz mannequin commented Jan 17, 2010

Attachment: trac7958_b.patch.gz

Attachment: trac7958_c.patch.gz

@sagetrac-spancratz
Copy link
Mannequin Author

sagetrac-spancratz mannequin commented Jan 17, 2010

comment:4

To see that this issue is now resolved (for rationals and number fields), consider
{{{sage: _. = ZZ[]
sage: K. = NumberField(x5-3x4+2424x^3+2x-232)
sage: R. = K.ring_of_integers()
sage: S. = R[]
sage: F = Frac(S)
sage: F(1/a)
a^4 - 3
a^3 + 2424a^2 + 2/232
sage: F(1/a).numerator()
a^4 - 3
a^3 + 2424*a^2 + 2
sage: F(1/a).denominator()
232
}}}

But the last three lines highlight a bug in the printing routines.

@mwhansen
Copy link
Contributor

comment:5

Attachment: trac7958_d.patch.gz


sage: _.<x> = ZZ[]
sage: K.<a> = NumberField(x^5-3*x^4+2424*x^3+2*x-232)
sage: R.<b> = K.ring_of_integers()
sage: S.<y> = R[]
sage: F = Frac(S)
sage: F(1/a)
a^4 - 3*a^3 + 2424*a^2 + 2/232
sage: F(1/a).numerator()
a^4 - 3*a^3 + 2424*a^2 + 2
sage: F(1/a).denominator()
232

@mwhansen
Copy link
Contributor

Combined version of the above patches.

@mwhansen
Copy link
Contributor

Attachment: trac7958.2.patch.gz

Attachment: trac_7958-atomic.patch.gz

@sagetrac-spancratz
Copy link
Mannequin Author

sagetrac-spancratz mannequin commented Jan 20, 2010

comment:6

Applying the two patches

  • trac7958.2.patch
  • trac_7958-atomic.patch

this applies cleanly and passes all doctests.

@sagetrac-spancratz
Copy link
Mannequin Author

sagetrac-spancratz mannequin commented Jan 20, 2010

Changed author from spancratz to Sebastian Pancratz, Mike Hansen

@sagetrac-spancratz
Copy link
Mannequin Author

sagetrac-spancratz mannequin commented Jan 20, 2010

Reviewer: Mike Hansen, Sebastian Pancratz

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jan 23, 2010

Merged: sage-4.3.2.alpha0

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jan 23, 2010

comment:7

Merged patches in this order:

  1. trac7958.2.patch
  2. trac_7958-atomic.patch

@sagetrac-mvngu sagetrac-mvngu mannequin closed this as completed Jan 23, 2010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants