Opened 8 years ago
Closed 5 weeks ago
#14602 closed defect (fixed)
Symbolic expression to number fields
Reported by:  vdelecroix  Owned by:  davidloeffler 

Priority:  minor  Milestone:  sage9.3 
Component:  number fields  Keywords:  
Cc:  vdelecroix, mkoeppe  Merged in:  
Authors:  Dave Morris  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  a5fc9f9 (Commits, GitHub, GitLab)  Commit:  a5fc9f94e2912ca1f6e9d1571e3ae6094b383059 
Dependencies:  Stopgaps: 
Description (last modified by )
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
Change History (21)
comment:1 Changed 8 years ago by
 Description modified (diff)
comment:2 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:3 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:4 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:5 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:6 Changed 5 years ago by
 Cc mkoeppe added
comment:7 Changed 20 months ago by
comment:8 Changed 20 months ago by
 Milestone changed from sage6.4 to sageduplicate/invalid/wontfix
 Status changed from new to needs_review
Indeed. The situation improved.
comment:9 Changed 20 months ago by
 Status changed from needs_review to positive_review
Yep.
Every example in the ticket text now works!
comment:10 followup: ↓ 11 Changed 20 months ago by
Is this doctested somewhere ?
comment:11 in reply to: ↑ 10 Changed 20 months ago by
 Status changed from positive_review to needs_work
Replying to chapoton:
Is this doctested somewhere ?
Not as far as I can tell. Good point. Forgot to check that.
comment:12 Changed 3 months ago by
 Branch set to public/14602
comment:13 Changed 3 months ago by
 Commit set to ac5aa6ac1a947815079faf94c91503f9ad897742
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:
ac5aa6a  doctests for trac 14602

comment:14 Changed 3 months ago by
 Milestone changed from sageduplicate/invalid/wontfix to sage9.3
 Priority changed from major to minor
 Status changed from needs_work to needs_review
comment:15 Changed 3 months ago by
Could you also doctest the other embedding
sage: L = QuadraticField(3, embedding=AA(3).sqrt()) sage: bool(L.gen() == sqrt(3)) True
comment:16 Changed 3 months ago by
 Commit changed from ac5aa6ac1a947815079faf94c91503f9ad897742 to a5fc9f94e2912ca1f6e9d1571e3ae6094b383059
Branch pushed to git repo; I updated commit sha1. New commits:
a5fc9f9  additional doctest

comment:17 Changed 3 months ago by
Thanks for the suggestion. I added this doctest.
comment:18 Changed 3 months ago by
 Reviewers set to Vincent Delecroix
Thanks. Good to me. Waiting for the patchbot.
comment:19 Changed 3 months ago by
 Status changed from needs_review to positive_review
comment:20 Changed 3 months ago by
Thanks!
comment:21 Changed 5 weeks ago by
 Branch changed from public/14602 to a5fc9f94e2912ca1f6e9d1571e3ae6094b383059
 Resolution set to fixed
 Status changed from positive_review to closed
everything seems to work fine now (sage 8.9.b7)