Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#7598 closed defect (fixed)

NumberField embedding slightly off

Reported by: mhansen Owned by: davidloeffler
Priority: major Milestone: sage-4.3
Component: number fields Keywords:
Cc: burcin Merged in: sage-4.3.rc1
Authors: William Stein Reviewers: Mike Hansen, John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

sage: Q.<i> = NumberField(x^2+1)
sage: complex(i)
0.99999999999999967j

It should give 1j instead.

Attachments (3)

trac_7598-more_serious_version.patch (16.5 KB) - added by was 10 years ago.
trac_7598-dirichlet.patch (1.5 KB) - added by mhansen 10 years ago.
trac_7598-nf_galois_groups.patch (1.3 KB) - added by AlexGhitza 10 years ago.
apply after the previous two patches

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by mhansen

  • Milestone set to sage-4.3

comment:2 Changed 10 years ago by burcin

  • Cc burcin added

comment:3 Changed 10 years ago by mhansen

This is probably caused by the roots method using NumPy? which uses Fortran which is a little off. If you specify algorithm='pari' to the roots command when computing them, things should be fine.

sage: Q.<i> = NumberField(x^2+1)
sage: Q.defining_polynomial().change_ring(CC).roots()[1][0].imag().exact_rational()
9007199254740989/9007199254740992
sage: Q.defining_polynomial().change_ring(ComplexField(100)).roots()[1][0].imag().exact_rational()
1

Note that NumPy? is not used for the second example.

Changed 10 years ago by was

comment:4 Changed 10 years ago by was

trac_7598-more_serious_version.patch -- this deals with the problems more at the root. Unfortunately, there are doctests in this file that fail:

	sage -t  devel/sage-main/sage/modular/dirichlet.py # 4 doctests failed

and I haven't had time to figure out what is wrong. It probably has to do with a complex embedding not being defined automatically, whereas before it was...

The design of embeddings was really bad before and relied on numerical errors to mess up the order of roots in case of 53 bit precision. This was potentially *very* buggy and was I think the result of some absolutely terrible design decisions. This absolutely must be fixed before releasing sage-4.3. This patch basically fixes it, modulo some small remaining issue.

Here is an example from sage-4.2.1 that illustrates just how horrendously bad the previous design was (with using CDF when prec=53 but ComplexField?(prec) otherwise):

sage: K.<i> = QuadraticField(-1)
sage: i.complex_em
i.complex_embedding   i.complex_embeddings  
sage: i.complex_embedding()
1.0*I
sage: i.complex_embedding(100)
-1.0000000000000000000000000000*I

comment:5 Changed 10 years ago by mhansen

I think the errors in dirichlet.py comes from the following:

sage: a = mod(1,3)
sage: CDF.zeta(3)^a
-0.5 + 0.866025403784*I
sage: CC.zeta(3)^a
---------------------------------------------------------------------------
Traceback (most recent call last)
...
TypeError: unable to coerce <type 'sage.rings.complex_number.ComplexNumber'> to an integer

It'd be nice if the {{{pow}} methods were standardized.

Changed 10 years ago by mhansen

comment:6 Changed 10 years ago by mhansen

  • Authors set to William Stein
  • Reviewers set to Mike Hansen
  • Status changed from new to needs_review

I've added a patch which fixes the errors in dirichlet.py by just forcing the exponent to be an int.

William's changes look good to me so the only thing that needs review is trac_7598-dirichlet.patch.

We should open another ticket for the __pow__ discrepencies.

comment:7 Changed 10 years ago by cremona

Do we need to apply both patches? Or is it that Mike has given a positive review to the first and only needs a review of the second? I was hoping to test this (being someone who experienced the problems) but as it will take a long time to rebuild, I want to make sure that the time is not wasted.

comment:8 Changed 10 years ago by mhansen

You need to apply both patches. I've given a positive review to the first. The second is pretty easy since it makes the exponent an integer instead of an integermod.

comment:9 Changed 10 years ago by AlexGhitza

  • Status changed from needs_review to needs_work

Running long doctests on a 32-bit Arch Linux machine gives one doctest failure related to this ticket:

sage -t -long "devel/sage/doc/en/bordeaux_2008/nf_galois_groups.rst"
**********************************************************************
File "/opt/sage-4.3.rc0/devel/sage/doc/en/bordeaux_2008/nf_galois_groups.rst", line 109:
    sage: K.complex_embeddings()
Expected:
    [
    Ring morphism:
      From: Number Field in a with defining polynomial x^3 - 2
      To:   Complex Double Field
      Defn: a |--> -0.629960524947 - 1.09112363597*I,
    Ring morphism:
      From: Number Field in a with defining polynomial x^3 - 2
      To:   Complex Double Field
      Defn: a |--> -0.629960524947 + 1.09112363597*I,
    Ring morphism:
      From: Number Field in a with defining polynomial x^3 - 2
      To:   Complex Double Field
      Defn: a |--> 1.25992104989
    ]
Got:
    [
    Ring morphism:
      From: Number Field in a with defining polynomial x^3 - 2
      To:   Complex Field with 53 bits of precision
      Defn: a |--> -0.629960524947437 - 1.09112363597172*I,
    Ring morphism:
      From: Number Field in a with defining polynomial x^3 - 2
      To:   Complex Field with 53 bits of precision
      Defn: a |--> -0.629960524947437 + 1.09112363597172*I,
    Ring morphism:
      From: Number Field in a with defining polynomial x^3 - 2
      To:   Complex Field with 53 bits of precision
      Defn: a |--> 1.25992104989487
    ]
**********************************************************************
1 items had failures:
   1 of   4 in __main__.example_3
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/ghitza/.sage//tmp/.doctest_nf_galois_groups.py
	 [5.9 s]
exit code: 1024

Apart from this small issue, everything looks good.

comment:10 Changed 10 years ago by AlexGhitza

OK, I've added a trivial patch that fixes the last doctest failure.

Changed 10 years ago by AlexGhitza

apply after the previous two patches

comment:11 Changed 10 years ago by AlexGhitza

  • Status changed from needs_work to needs_review

comment:12 Changed 10 years ago by cremona

  • Reviewers changed from Mike Hansen to Mike Hansen, John Cremona
  • Status changed from needs_review to positive_review

Applied all 3 patches to 4.3.rc0 on both 32-bit Suse (built using system gfortran) and 64-bit ubuntu (using Sage's gfortran). No problems. All tests in the files touched pass. I still get a failure in doc/en/bordeaux_2008/nf_introduction.rst but that has a different cause.

So, positive review.

comment:13 Changed 10 years ago by mhansen

Excellent. I'm glad we (you guys) tracked that down. I'm not sure why I didn't think that the number field was just using the alternate embedding.

comment:14 Changed 10 years ago by mhansen

  • Merged in set to sage-4.3.rc1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:15 Changed 10 years ago by mhansen

  • Milestone changed from sage-4.3.1 to sage-4.3
Note: See TracTickets for help on using tickets.