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: sage-9.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:

Status badges

Description (last modified by vdelecroix)

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 vdelecroix

  • Description modified (diff)

comment:2 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:3 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:4 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:5 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:6 Changed 5 years ago by mkoeppe

  • Cc mkoeppe added

comment:7 Changed 20 months ago by chapoton

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

comment:8 Changed 20 months ago by vdelecroix

  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

Indeed. The situation improved.

comment:9 Changed 20 months ago by klui

  • Status changed from needs_review to positive_review

Yep.

Every example in the ticket text now works!

comment:10 follow-up: Changed 20 months ago by chapoton

Is this doctested somewhere ?

comment:11 in reply to: ↑ 10 Changed 20 months ago by klui

  • 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 gh-DaveWitteMorris

  • Branch set to public/14602

comment:13 Changed 3 months ago by gh-DaveWitteMorris

  • 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:

ac5aa6adoctests for trac 14602

comment:14 Changed 3 months ago by gh-DaveWitteMorris

  • Authors set to Dave Morris
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-9.3
  • Priority changed from major to minor
  • Status changed from needs_work to needs_review

comment:15 Changed 3 months ago by vdelecroix

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 git

  • Commit changed from ac5aa6ac1a947815079faf94c91503f9ad897742 to a5fc9f94e2912ca1f6e9d1571e3ae6094b383059

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

a5fc9f9additional doctest

comment:17 Changed 3 months ago by gh-DaveWitteMorris

Thanks for the suggestion. I added this doctest.

comment:18 Changed 3 months ago by vdelecroix

  • Reviewers set to Vincent Delecroix

Thanks. Good to me. Waiting for the patchbot.

comment:19 Changed 3 months ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:20 Changed 3 months ago by gh-DaveWitteMorris

Thanks!

comment:21 Changed 5 weeks ago by vbraun

  • Branch changed from public/14602 to a5fc9f94e2912ca1f6e9d1571e3ae6094b383059
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.