Skip to content

Symbolic expression to number fields #14602

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

Closed
videlec opened this issue May 17, 2013 · 18 comments
Closed

Symbolic expression to number fields #14602

videlec opened this issue May 17, 2013 · 18 comments

Comments

@videlec
Copy link
Contributor

videlec commented May 17, 2013

The ticket stands to improve the AlgebraicConverter in sage.symbolic.expression_converters and make it works with number fields.

As mentioned on ask the following fails

sage: K = QuadraticField(3)
sage: K(sqrt(3))
Traceback (most recent call last):
...
TypeError: ...

The following gives an answer with a wrong parent

sage: x = K(3)**(1/2); x
sqrt(3)
sage: a.parent()
Symbolic Ring

while it is possible to do

sage: y = K(3).sqrt(); y
a
sage: y == K.gen()
True

Finally, we hopefully have

sage: K.gen() == sqrt(3)
sqrt(3) == sqrt(3)
sage: bool(K.gen() == sqrt(3))
True

CC: @videlec @mkoeppe

Component: number fields

Author: Dave Morris

Branch/Commit: a5fc9f9

Reviewer: Vincent Delecroix

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

@videlec

This comment has been minimized.

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@fchapoton
Copy link
Contributor

comment:7

everything seems to work fine now (sage 8.9.b7)

@videlec
Copy link
Contributor Author

videlec commented Aug 24, 2019

comment:8

Indeed. The situation improved.

@videlec videlec removed this from the sage-6.4 milestone Aug 24, 2019
@kevinywlui
Copy link
Mannequin

kevinywlui mannequin commented Aug 25, 2019

comment:9

Yep.

Every example in the ticket text now works!

@fchapoton
Copy link
Contributor

comment:10

Is this doctested somewhere ?

@kevinywlui
Copy link
Mannequin

kevinywlui mannequin commented Aug 25, 2019

comment:11

Replying to @fchapoton:

Is this doctested somewhere ?

Not as far as I can tell. Good point. Forgot to check that.

@DaveWitteMorris
Copy link
Member

Branch: public/14602

@DaveWitteMorris
Copy link
Member

comment:13

The PR adds doctests. Note that we still have

sage: (K(3)^(1/2)).parent()                                                  
Symbolic Ring

That seems ok to me, but if the parent should be K, then I think another ticket should be opened. (The parent of sqrt(K(3)) is K, and is one of the new doctests.)


New commits:

ac5aa6adoctests for trac 14602

@DaveWitteMorris
Copy link
Member

Commit: ac5aa6a

@DaveWitteMorris
Copy link
Member

Author: Dave Morris

@DaveWitteMorris DaveWitteMorris added this to the sage-9.3 milestone Jan 24, 2021
@videlec
Copy link
Contributor Author

videlec commented Jan 24, 2021

comment:15

Could you also doctest the other embedding

sage: L = QuadraticField(3, embedding=-AA(3).sqrt())
sage: bool(L.gen() == -sqrt(3))
True

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 24, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

a5fc9f9additional doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 24, 2021

Changed commit from ac5aa6a to a5fc9f9

@DaveWitteMorris
Copy link
Member

comment:17

Thanks for the suggestion. I added this doctest.

@videlec
Copy link
Contributor Author

videlec commented Jan 24, 2021

Reviewer: Vincent Delecroix

@videlec
Copy link
Contributor Author

videlec commented Jan 24, 2021

comment:18

Thanks. Good to me. Waiting for the patchbot.

@DaveWitteMorris
Copy link
Member

comment:20

Thanks!

@vbraun
Copy link
Member

vbraun commented Mar 9, 2021

Changed branch from public/14602 to a5fc9f9

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

5 participants