Opened 4 years ago

Closed 7 months ago

Last modified 7 months ago

#20181 closed enhancement (fixed)

number_field_elements_from_algebraics should create embedded number field elements

Reported by: mkoeppe Owned by:
Priority: minor Milestone: sage-8.8
Component: number fields Keywords: IMA-PolyGeom
Cc: vbraun, gagern, vdelecroix, tmonteil, jj, sstarosta, jipilab, moritz, yzh, cremona, tscrim Merged in:
Authors: Jean-Philippe Labbé Reviewers: Matthias Koeppe, John Cremona
Report Upstream: N/A Work issues:
Branch: 9255f2c (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jipilab)

This is a follow-up on #17830 and #18819.

#17830 made this possible:

sage: x = polygen(ZZ)
sage: K.<cbrt2> = NumberField(x^3 - 2, embedding=AA.polynomial_root(x^3-2, RIF(0,3)))
sage: 6064/4813 < cbrt2 < 90325/71691
True

But if one uses number_field_elements_from_algebraics, one gets only non-embedded number field elements:

sage: x = polygen(ZZ)
sage: K, cbrt2, hom = number_field_elements_from_algebraics(AA.polynomial_root(x^3-2, RIF(0,3)))
sage: 6064/4813 < cbrt2 < 90325/71691
False            # (an perhaps an error in Python 3?)

This ticket adds an option embedded to create embedded number field elements.

sage: x = polygen(ZZ)
sage: K, cbrt2, hom = number_field_elements_from_algebraics(AA.polynomial_root(x^3-2, RIF(0,3)), embedded=True)
sage: 6064/4813 < cbrt2 < 90325/71691
True

TODECIDE:

Change History (55)

comment:1 Changed 4 years ago by mkoeppe

While looking at related tickets, I noticed the very old ticket #5355, which should be closed I think.

comment:2 Changed 4 years ago by mkoeppe

  • Description modified (diff)

comment:3 Changed 21 months ago by mkoeppe

  • Cc jipilab moritz added

comment:4 Changed 20 months ago by mkoeppe

  • Cc yzh added

comment:5 Changed 20 months ago by jipilab

  • Branch set to public/algnum_to_numberfield
  • Commit set to 26dc3e4e6a26f6f613a69d57929ea492c278dad0
  • Status changed from new to needs_info

Here is a first version. Comments are welcome.

One question is whether RealIntervalField is the right choice?


New commits:

26dc3e4Added option for embedded NumberField

comment:6 Changed 20 months ago by jipilab

Maybe it would be nice to also add the optional parameter to the method as_number_field_element as well...

comment:7 Changed 20 months ago by jipilab

The following should work but it does not:

sage: UCF = UniversalCyclotomicField()
sage: E = UCF.gen(5)
sage: my_nums = [-52*E - 136*E^2 - 136*E^3 - 52*E^4, -66*E - 168*E^2 - 168*E^3 - 66*E^4, -46*E - 114*E^2 - 114*E^3 - 46*E^4, -24*E - 58*E^2 - 58*E
....: ^3 - 24*E^4, sqrt(2)]
sage: [n(_) for _ in my_nums]
[187.914855054991 - 8.67361737988404e-18*I,
 231.039466852489 - 1.04083408558608e-17*I,
 156.026311234993 - 8.67361737988404e-18*I,
 79.0131556174964 - 4.33680868994202e-18*I,
 1.41421356237310]
sage: res = number_field_elements_from_algebraics(my_nums,embedded=True)
Traceback (most recent call last):
...
TypeError: unable to convert -0.4370160244488211? - 1.344997023927915?*I to real interval

The choice of root shoould always be made real...

Last edited 20 months ago by jipilab (previous) (diff)

comment:8 Changed 20 months ago by git

  • Commit changed from 26dc3e4e6a26f6f613a69d57929ea492c278dad0 to db778eeeb93978335aa113e09a51ccabfe2350e4

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

db778eeMade it possible to deal with complex number fields

comment:9 Changed 20 months ago by jipilab

It is now possible to get this:

sage: UCF = UniversalCyclotomicField()
sage: E = UCF.gen(5)
sage: L.<b> = NumberField(x^2-189*x+16, embedding=200)
sage: my_nums = [-52*E - 136*E^2 - 136*E^3 - 52*E^4, L.gen()._algebraic_(AA),sqrt(2)]
sage: aa_my_nums = [AA(_) for _ in my_nums]
sage: res = number_field_elements_from_algebraics(aa_my_nums,embedded=True)
sage: res
(Number Field in a with defining polynomial y^8 - 35670*y^6 + 476899047*y^4 - 2832410271650*y^2 + 6305298701739921,
 [2310/26212773509*a^7 - 185432947/78638320527*a^5 + 1652517502195/78638320527*a^3 - 4904676315215467/78638320527*a + 94,
  -1238/2803377488467023*a^7 + 185460719/11213509953868092*a^5 - 2754936849443/11213509953868092*a^3 + 8180694680816975/3737836651289364*a + 189/2,
  -1979/1887160880826*a^7 + 26472586/943580440413*a^5 - 235822245043/943580440413*a^3 + 466325019915415/629053626942*a],
 Ring morphism:
   From: Number Field in a with defining polynomial y^8 - 35670*y^6 + 476899047*y^4 - 2832410271650*y^2 + 6305298701739921
   To:   Algebraic Real Field
   Defn: a |--> 96.9475535136628?)
sage: res[0].gen_embedding()
96.9475535136628?
sage: res = number_field_elements_from_algebraics(aa_my_nums,embedded=False)
sage: res[0].gen_embedding()  # Nothing is returned(!?!?!)
sage:

Perhaps it would be nice to say that there is no embedding instead of returning nothing?

comment:10 Changed 20 months ago by jipilab

  • Status changed from needs_info to needs_review

comment:11 Changed 20 months ago by jipilab

  • Authors set to Jean-Philippe Labbé

comment:12 follow-up: Changed 20 months ago by mkoeppe

The returned morphism is not able to convert the elements.

sage: res[2](my_nums[0])
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-8-74370e91d982> in <module>()
----> 1 res[Integer(2)](my_nums[Integer(0)])

/Users/mkoeppe/s/sage/some-sage-install-prefix/lib/python2.7/site-packages/sage/categories/map.pyx in sage.categories.map.Map.__call__ (build/cythonized/sage/categories/map.c:6978)()
    781                 x = D(x)
    782             except (TypeError, NotImplementedError):
--> 783                 raise TypeError("%s fails to convert into the map's domain %s, but a `pushforward` method is not properly implemented" % (x, D))
    784         else:
    785             x = converter(x)

TypeError: -52*E(5) - 136*E(5)^2 - 136*E(5)^3 - 52*E(5)^4 fails to convert into the map's domain Number Field in a with defining polynomial y^8 - 35670*y^6 + 476899047*y^4 - 2832410271650*y^2 + 6305298701739921, but a `pushforward` method is not properly implemented

Edit: I withdraw this comment; I misremembered what hom is supposed to do.

Last edited 20 months ago by mkoeppe (previous) (diff)

comment:13 follow-up: Changed 20 months ago by mmezzarobba

Two quick comments:

  • you may want to pathc as_number_field_element() too,
  • don't forget to add tests covering the case where some or all of the elements are rationals or elements of quadratic extensions (rationals, quadratic number field elements and higher-degree number field elements don't have exactly the same interface...)

comment:14 in reply to: ↑ 13 Changed 20 months ago by jipilab

Replying to mmezzarobba:

Two quick comments:

  • you may want to pathc as_number_field_element() too,
  • don't forget to add tests covering the case where some or all of the elements are rationals or elements of quadratic extensions (rationals, quadratic number field elements and higher-degree number field elements don't have exactly the same interface...)

Yes, that sounds very reasonable. I will do that, along with fixing what mkoeppe mentionned above.

comment:15 Changed 20 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:16 Changed 20 months ago by mkoeppe

Doesn't always seem to give real embeddings:

sage: x = polygen(ZZ); elts = number_field_elements_from_algebraics([sqrt(2), AA.polynomial_root(x^3-2, RIF(0,3))], embedded=True)[1]
sage: [AA(y) for y in elts]
...
TypeError: unable to convert -0.561231024154687? - 0.9720806486198328?*I to real interval

comment:17 in reply to: ↑ 12 Changed 20 months ago by jipilab

Replying to mkoeppe:

The returned morphism is not able to convert the elements.

sage: res[2](my_nums[0])
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-8-74370e91d982> in <module>()
----> 1 res[Integer(2)](my_nums[Integer(0)])

/Users/mkoeppe/s/sage/some-sage-install-prefix/lib/python2.7/site-packages/sage/categories/map.pyx in sage.categories.map.Map.__call__ (build/cythonized/sage/categories/map.c:6978)()
    781                 x = D(x)
    782             except (TypeError, NotImplementedError):
--> 783                 raise TypeError("%s fails to convert into the map's domain %s, but a `pushforward` method is not properly implemented" % (x, D))
    784         else:
    785             x = converter(x)

TypeError: -52*E(5) - 136*E(5)^2 - 136*E(5)^3 - 52*E(5)^4 fails to convert into the map's domain Number Field in a with defining polynomial y^8 - 35670*y^6 + 476899047*y^4 - 2832410271650*y^2 + 6305298701739921, but a `pushforward` method is not properly implemented

The homomorphism goes the other way around... Thanks to Moritz for noticing that.

comment:18 Changed 20 months ago by git

  • Commit changed from db778eeeb93978335aa113e09a51ccabfe2350e4 to a9045bf8a3aab2d0aa00be17e91227bc1b50262a

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

a9045bfAdded tests and made them pass

comment:19 Changed 20 months ago by jipilab

Okay. I added some tests and now it seems to work.

With the current commit, I have one failing doctest for which I do not know what to do:

File "qqbar.py", line 2098, in sage.rings.qqbar.number_field_elements_from_algebraics
Failed example:
    number_field_elements_from_algebraics(rt2c)
Expected:
    (Number Field in a with defining polynomial y^4 + 2*y^2 + 4, 1/2*a^3, Ring morphism:
        From: Number Field in a with defining polynomial y^4 + 2*y^2 + 4
        To:   Algebraic Field
        Defn: a |--> -0.7071067811865475? - 1.224744871391589?*I)
Got:
    (Number Field in a with defining polynomial y^2 - 2, a, Ring morphism:
       From: Number Field in a with defining polynomial y^2 - 2
       To:   Algebraic Real Field
       Defn: a |--> 1.414213562373095?)

This is in the tests section and the explanation given is exactly a situation that we have to outsmart when we want to be able to accept cyclotomic fields elements to be parsed to real embedded number fields.

It seems to me that the proposed code improves the situation as Sage now recognize that it is a real number... I can not see a reason right now to go against that. Am I missing something?

comment:20 Changed 20 months ago by jipilab

  • Status changed from needs_work to needs_review

comment:21 Changed 20 months ago by jipilab

  • Keywords IMA-PolyGeom added

comment:22 Changed 20 months ago by dimpase

  • Milestone changed from sage-7.1 to sage-8.3

comment:23 follow-up: Changed 20 months ago by dimpase

  • Cc cremona added

Modifying the test in comment 19 looks OK to me, but perhaps John can confirm I'm not missing anything...

comment:24 Changed 20 months ago by chapoton

You should just go on and fix the doctest, I think.

comment:25 Changed 20 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to needs_work

as_number_field_element needs to pass on its new arguments, and corresponding doctests should be added.

comment:26 Changed 15 months ago by git

  • Commit changed from a9045bf8a3aab2d0aa00be17e91227bc1b50262a to 60c527f8ef3f2ed7bb75220fceeb532ff7096120

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

60c527fMerge remote-tracking branch 'trac/develop' into t/20181/public/algnum_to_numberfield

comment:27 Changed 15 months ago by git

  • Commit changed from 60c527f8ef3f2ed7bb75220fceeb532ff7096120 to f1627e7a51c74327cfb91e37eaecadd6b3aee173

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

f1627e7AlgebraicNumber_base.as_number_field_element: Pass new kwargs to number_field_elements_from_algebraics

comment:28 Changed 15 months ago by mkoeppe

... still needs a corresponding doctest

comment:29 Changed 15 months ago by git

  • Commit changed from f1627e7a51c74327cfb91e37eaecadd6b3aee173 to 41b1142066d3c718006ee3363d41f4367949bc03

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

d7e31a2AlgebraicNumber_base.as_number_field_element: Add doctest
284c217Merge remote-tracking branch 'trac/develop' into t/20181/public/algnum_to_numberfield
41b1142AlgebraicNumber_base.as_number_field_element: Add doctests (currently failing)

comment:30 Changed 15 months ago by mkoeppe

I've added some doctests, but the implementation needs more work as there are failures.

comment:31 Changed 15 months ago by mkoeppe

  • Cc tscrim added
  • Description modified (diff)
  • Milestone changed from sage-8.3 to sage-8.4

comment:32 Changed 15 months ago by git

  • Commit changed from 41b1142066d3c718006ee3363d41f4367949bc03 to 2eb332053089cccf9266850d2fbd964a3c24b9fe

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

2eb3320number_field_elements_from_algebraics: Mark a random doctest # random

comment:33 in reply to: ↑ 23 Changed 15 months ago by mkoeppe

Replying to dimpase:

Modifying the test in comment 19 looks OK to me, but perhaps John can confirm I'm not missing anything...

The context of this doctest suggested that it should be marked # random, which I did in 2eb3320.

comment:34 Changed 15 months ago by tscrim

Ready for re-review?

Some quick comments. It is considered a bad practice to have bare except: statements. It is probably sufficient to catch ValueError and TypeError. Also, the standard doc format for INPUT: would be

    - ``numbers`` -- a number or list of numbers

    - ``minimal`` -- boolean (default: ``False``); whether to minimize the
      degree of the extension

    - ``same_field`` -- boolean (default: ``False``); see below

    - ``embedded`` -- boolean (default: ``False``); whether to make the
      :func:`NumberField` embedded

    - ``prec`` -- integer (default: ``53``); the number of bit of precision for
      the embedding
        - ``minimal`` -- boolean (default: ``False``); whether to minimize the
          degree of the extension

        - ``embedded`` -- boolean (default: ``False``); whether to make the
          :func:`NumberField` embedded
    
        - ``prec`` -- integer (default: ``53``); the number of bit of precision for
          the embedding

comment:35 follow-up: Changed 15 months ago by mkoeppe

Not ready for review; this needs more work -- see the failing doctests that I added in 41b1142.

comment:36 in reply to: ↑ 35 Changed 15 months ago by mkoeppe

Replying to mkoeppe:

Not ready for review; this needs more work -- see the failing doctests that I added in 41b1142.

For reference: These are the doctest failures.

$ sage -t src/sage/rings/qqbar.py 
too many failed tests, not using stored timings
Running doctests with ID 2018-09-27-16-59-10-65572ec7.
Using --optional=4ti2,bliss,ccache,dochtml,gdb,latte_int,lidia,lrslib,mpir,normaliz,notedown,pandoc_attributes,pynormaliz,pyqnormaliz,python2,rst2ipynb,sage
Doctesting 1 file.
sage -t src/sage/rings/qqbar.py
**********************************************************************
File "src/sage/rings/qqbar.py", line 3696, in sage.rings.qqbar.AlgebraicNumber_base.as_number_field_element
Failed example:
    elt == rt
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage/rings/qqbar.py", line 3698, in sage.rings.qqbar.AlgebraicNumber_base.as_number_field_element
Failed example:
    AA(elt)
Exception raised:
    Traceback (most recent call last):
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 659, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1070, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.qqbar.AlgebraicNumber_base.as_number_field_element[12]>", line 1, in <module>
        AA(elt)
      File "sage/structure/parent.pyx", line 921, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9679)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 145, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4574)
        raise
      File "sage/structure/coerce_maps.pyx", line 140, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4442)
        return C._element_constructor(x)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/local/lib/python2.7/site-packages/sage/rings/qqbar.py", line 738, in _element_constructor_
        return x._algebraic_(AA)
      File "sage/rings/number_field/number_field_element.pyx", line 2709, in sage.rings.number_field.number_field_element.NumberFieldElement._algebraic_ (build/cythonized/sage/rings/number_field/number_field_element.cpp:26930)
        emb = refine_embedding(emb, infinity)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/local/lib/python2.7/site-packages/sage/rings/number_field/number_field.py", line 11329, in refine_embedding
        diffs = [(RC(ee(K.gen()))-old_root).abs() for ee in elist]
      File "sage/rings/real_mpfi.pyx", line 699, in sage.rings.real_mpfi.RealIntervalField_class.__call__ (build/cythonized/sage/rings/real_mpfi.c:6094)
        return Parent.__call__(self, x)
      File "sage/structure/parent.pyx", line 921, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9679)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 271, in sage.structure.coerce_maps.NamedConvertMap._call_ (build/cythonized/sage/structure/coerce_maps.c:5999)
        cdef Element e = method(C)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/local/lib/python2.7/site-packages/sage/rings/qqbar.py", line 4007, in interval
        raise TypeError("unable to convert {} to real interval".format(self))
    TypeError: unable to convert -2.618826569900552? - 0.2237500597843041?*I to real interval
**********************************************************************
File "src/sage/rings/qqbar.py", line 3700, in sage.rings.qqbar.AlgebraicNumber_base.as_number_field_element
Failed example:
    RR(elt)
Exception raised:
    Traceback (most recent call last):
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 659, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1070, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.qqbar.AlgebraicNumber_base.as_number_field_element[13]>", line 1, in <module>
        RR(elt)
      File "sage/structure/parent.pyx", line 921, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9679)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 271, in sage.structure.coerce_maps.NamedConvertMap._call_ (build/cythonized/sage/structure/coerce_maps.c:5993)
        cdef Element e = method(C)
      File "sage/rings/number_field/number_field_element.pyx", line 1802, in sage.rings.number_field.number_field_element.NumberFieldElement._mpfr_ (build/cythonized/sage/rings/number_field/number_field_element.cpp:19625)
        return R(R.complex_field()(self))
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/local/lib/python2.7/site-packages/sage/rings/complex_field.py", line 381, in __call__
        return Parent.__call__(self, x)
      File "sage/structure/parent.pyx", line 921, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9679)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 145, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4574)
        raise
      File "sage/structure/coerce_maps.pyx", line 140, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4442)
        return C._element_constructor(x)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/local/lib/python2.7/site-packages/sage/rings/complex_field.py", line 423, in _element_constructor_
        return ComplexNumber(self, x)
      File "sage/rings/complex_number.pyx", line 200, in sage.rings.complex_number.ComplexNumber.__init__ (build/cythonized/sage/rings/complex_number.c:4535)
        rr = R(real)
      File "sage/structure/parent.pyx", line 921, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9679)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 271, in sage.structure.coerce_maps.NamedConvertMap._call_ (build/cythonized/sage/structure/coerce_maps.c:5993)
        cdef Element e = method(C)
      File "sage/rings/number_field/number_field_element.pyx", line 1802, in sage.rings.number_field.number_field_element.NumberFieldElement._mpfr_ (build/cythonized/sage/rings/number_field/number_field_element.cpp:19625)
        return R(R.complex_field()(self))
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/local/lib/python2.7/site-packages/sage/rings/complex_field.py", line 381, in __call__
        return Parent.__call__(self, x)
      File "sage/structure/parent.pyx", line 921, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9679)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 145, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4574)
        raise
      File "sage/structure/coerce_maps.pyx", line 140, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4442)
.....................
      File "sage/categories/map.pyx", line 176, in sage.categories.map.Map.__copy__ (build/cythonized/sage/categories/map.c:4133)
        cdef Map out = Element.__copy__(self)
      File "sage/structure/element.pyx", line 605, in sage.structure.element.Element.__copy__ (build/cythonized/sage/structure/element.c:5273)
        D = self.__dict__
      File "sage/structure/element.pyx", line 493, in sage.structure.element.Element.__getattr__ (build/cythonized/sage/structure/element.c:4550)
        return self.getattr_from_category(name)
      File "sage/structure/element.pyx", line 506, in sage.structure.element.Element.getattr_from_category (build/cythonized/sage/structure/element.c:4659)
        return getattr_from_other_class(self, cls, name)
      File "sage/cpython/getattr.pyx", line 386, in sage.cpython.getattr.getattr_from_other_class (build/cythonized/sage/cpython/getattr.c:2428)
        if isinstance(self, cls):
    RuntimeError: maximum recursion depth exceeded while calling a Python object
**********************************************************************
1 item had failures:
   3 of  21 in sage.rings.qqbar.AlgebraicNumber_base.as_number_field_element
    [1438 tests, 3 failures, 14.69 s]
----------------------------------------------------------------------
sage -t src/sage/rings/qqbar.py  # 3 doctests failed
----------------------------------------------------------------------

For the infinite recursion in the last failure, perhaps could someone experienced with the Sage coercion system help?

comment:37 follow-up: Changed 15 months ago by tscrim

So I think I know what is going wrong. The issue is that there is no coercion from RIF -> RR; it goes the other way around. So with these changes, I can avoid the infinite recursion with RR(elt), but I get other failures:

  • src/sage/rings/qqbar.py

    diff --git a/src/sage/rings/qqbar.py b/src/sage/rings/qqbar.py
    index b32aa1a..7453d89 100644
    a b def number_field_elements_from_algebraics(numbers, minimal=False, same_field=False, embedded=False, prec=53): 
    21972197
    21982198    # Make the numbers have a real root
    21992199    real_numbers = []
     2200    from sage.rings.real_mpfr import RealField
    22002201    for v in numbers:
    22012202        if v._exact_field().is_complex() and real_case:
    22022203            # the number comes from a complex algebraic number field
    2203             embedded_rt = v.interval_fast(RealIntervalField(prec))
     2204            embedded_rt = v.interval_fast(RealField(prec))
    22042205            root = ANRoot(v.minpoly(), embedded_rt)
    22052206            real_nf = NumberField(v.minpoly(),'a')
    22062207            new_ef = AlgebraicGenerator(real_nf, root)
    def number_field_elements_from_algebraics(numbers, minimal=False, same_field=False, embedded=False, prec=53): 
    22232224    if fld is not QQ and embedded:
    22242225        assert real_case
    22252226        exact_generator = hom(fld.gen(0))
    2226         embedding_field = RealIntervalField(prec)
     2227        embedding_field = RealField(prec)
    22272228        embedded_gen = embedding_field(exact_generator)
    22282229        embedded_field = NumberField(fld.defining_polynomial(),fld.variable_name(),embedding=embedded_gen)
    2229        
    22302230        inter_hom = fld.hom([embedded_field.gen(0)])
    22312231        nums = map(inter_hom, nums)
    22322232        hom = embedded_field.hom([gen.root_as_algebraic()])

Note that with your current branch, the replacement of the RR(elt) by RIF(elt) works.

The reason you get the infinite recursion is somewhere in the number field coercion (conversion?)( code, it is trying to construct a complex number, which then tries to create the RR, but it cannot do that, so it tried to construct a number field element, and hence the cycle. Or something like that. The reason it normally doesn't cycle is because it separately knows how to get the the information it needs (if it even falls into this cycle). I don't completely understand the mechanism, but this is what the traceback is suggesting to me.

comment:38 in reply to: ↑ 37 Changed 15 months ago by mmezzarobba

Replying to tscrim:

So I think I know what is going wrong. The issue is that there is no coercion from RIF -> RR; it goes the other way around.

Note that #15114 is for removing this coercion, but is currently blocked by more or less the same issues with comparisons between elements of different parents that need to be solved for the Python 3 transition.

comment:39 Changed 9 months ago by jipilab

  • Description modified (diff)

comment:40 Changed 9 months ago by jipilab

Just as a summary here are things that need to be done (maybe some of them are already done):

  • you may want to patch as_number_field_element() too,
  • don't forget to add tests covering the case where some or all of the elements are rationals or elements of quadratic extensions (rationals, quadratic number field elements and higher-degree number field elements don't have exactly the same interface...)
  • Fix doctest in File "qqbar.py", line 2098, in sage.rings.qqbar.number_field_elements_from_algebraics
  • as_number_field_element needs to pass on its new arguments, and corresponding doctests should be added.
  • Fix Comment 34 and Comment 36.

comment:41 Changed 8 months ago by git

  • Commit changed from 2eb332053089cccf9266850d2fbd964a3c24b9fe to 5793bcfce6f66f5740fce05c4d89c1587eaee872

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

5793bcfMerge branch 'public/algnum_to_numberfield' of trac.sagemath.org:sage into 20181

comment:42 Changed 8 months ago by jipilab

  • Milestone changed from sage-8.4 to sage-8.8

New commits:

5793bcfMerge branch 'public/algnum_to_numberfield' of trac.sagemath.org:sage into 20181

New commits:

5793bcfMerge branch 'public/algnum_to_numberfield' of trac.sagemath.org:sage into 20181

comment:43 Changed 8 months ago by jipilab

While rereading the docstring, I realized that it is quite confusing. Especially the following lines:

2154     Note that for the first example, where \sage does not realize that 
2155     the number is real, we get a homomorphism to ``QQbar``; but with
2156     ``minimal=True``, we get a homomorphism to ``AA``.  If we specify
2157     both ``minimal=True`` and ``same_field=True``, we get a second
2158     degree extension (minimal) that maps back to ``QQbar``.

comment:44 Changed 8 months ago by git

  • Commit changed from 5793bcfce6f66f5740fce05c4d89c1587eaee872 to cb63fe51cda3ac27a67c297e9a05289b2911c865

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

cb63fe5Added some doctest for embedded case

comment:45 Changed 8 months ago by git

  • Commit changed from cb63fe51cda3ac27a67c297e9a05289b2911c865 to b913dcc0bc20886e637ffe2e05a88907f8eccdf2

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

b913dccFixed usage of RealField and docstring

comment:46 Changed 8 months ago by git

  • Commit changed from b913dcc0bc20886e637ffe2e05a88907f8eccdf2 to b153ef700dd3bab9966283e69ae24afc3bbf2245

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

b153ef7fixed docstring

comment:47 Changed 8 months ago by git

  • Commit changed from b153ef700dd3bab9966283e69ae24afc3bbf2245 to 61e4df9fabf9e06a21d75eb6320ab3179bc28096

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

61e4df9Fix docstring even more

comment:48 Changed 8 months ago by jipilab

  • Status changed from needs_work to needs_review

I deleted an old comment that should have been deleted in 2010 from ticket #9343.

It now looks ready for review.

comment:49 Changed 8 months ago by git

  • Commit changed from 61e4df9fabf9e06a21d75eb6320ab3179bc28096 to 9255f2c89588c79de496660c8c9030f087b5ebfd

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

9255f2cpyflakes

comment:50 Changed 8 months ago by mkoeppe

Looks good to me and works well within #25097. But it would be good if another reviewer could take a look as well.

Last edited 8 months ago by mkoeppe (previous) (diff)

comment:51 Changed 8 months ago by dimpase

It would be great is a semi-full-time number theorist looks into this...

comment:52 Changed 8 months ago by cremona

I almost never use embedded number fields (instead I make explicit embeddings into RR or CC as needed), but this all looks reasonable and there is a lot of new documentation and examples, which is good. So OK from me.

comment:53 Changed 8 months ago by mkoeppe

  • Reviewers changed from Matthias Koeppe to Matthias Koeppe, John Cremona
  • Status changed from needs_review to positive_review

comment:54 Changed 7 months ago by vbraun

  • Branch changed from public/algnum_to_numberfield to 9255f2c89588c79de496660c8c9030f087b5ebfd
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:55 Changed 7 months ago by chapoton

  • Commit 9255f2c89588c79de496660c8c9030f087b5ebfd deleted

you used "map" in a way that is not compatible with py3, please review the fix in #27767

Note: See TracTickets for help on using tickets.