#15027 closed defect (fixed)

Inconsistent primitive_root_of_unity for number fields

Reported by: cremona Owned by: cremona
Priority: major Milestone: sage-5.13
Component: number fields Keywords: number field unit
Cc: Merged in: sage-5.13.beta3
Authors: John Cremona, Jeroen Demeyer Reviewers: Peter Bruin
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

In 5.11.rc0:

sage: K = QuadraticField(-3)
sage: K.primitive_root_of_unity()    
1/2*a + 1/2
sage: UK = K.unit_group()
sage: K.primitive_root_of_unity()           
u
sage: K.primitive_root_of_unity().value()     
-1/2*a + 1/2

In the first call since the unit group has not yet been computed and cached, a fundamental torsion unit is computed idrectly (using pari). In the second case it is obtained from the cached unit group, but should have value() applied to convert from an abstract element of the unit group to an actual number field element.

Attachments (1)

trac15027-unit.patch (9.2 KB) - added by jdemeyer 10 months ago.
Apply to 5.11.rc0

Download all attachments as: .zip

Change History (15)

comment:1 Changed 13 months ago by cremona

Patch fixes the problem, now testing.

comment:2 Changed 13 months ago by cremona

  • Authors set to John Cremona
  • Cc pbruin added
  • Owner set to cremona

comment:3 follow-up: Changed 13 months ago by fwclarke

John,

The patch doesn't seem to eliminate the other inconsistency: between -1/2*a + 1/2 and 1/2*a + 1/2. For when primitive_root_of_unity is called before unit_group it selects the primitive with the shortest string representation. On the other hand, UnitGroup.__init__ just uses the element given by Pari's nfrootsof1. There are clearly a variety of ways to sort this out.

comment:4 in reply to: ↑ 3 Changed 13 months ago by cremona

Replying to fwclarke:

John,

The patch doesn't seem to eliminate the other inconsistency: between -1/2*a + 1/2 and 1/2*a + 1/2. For when primitive_root_of_unity is called before unit_group it selects the primitive with the shortest string representation. On the other hand, UnitGroup.__init__ just uses the element given by Pari's nfrootsof1. There are clearly a variety of ways to sort this out.

Fair comment, but once you have computed the unit group it's quite important to go with its generators or discrete logs get confused. The shortest-string business (which I wrote, though I cannot quite remember the issue which led me to) is less important. But what was really needed was not to have the root of unity displaying on the lmfdb.org page of a number field as "u0"!

comment:5 Changed 12 months ago by cremona

  • Status changed from new to needs_review

comment:6 follow-up: Changed 12 months ago by jdemeyer

  • Status changed from needs_review to needs_work

TESTS:: should be TESTS:

comment:7 in reply to: ↑ 6 Changed 12 months ago by cremona

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

TESTS:: should be TESTS:

--fixed.

comment:8 Changed 12 months ago by fwclarke

  • Status changed from needs_review to needs_work

Here's a strange thing. If I apply the patch (to 5.11), build and test, everything is fine. But if I restart Sage and type

sage: K = QuadraticField(-3)
sage: K.primitive_root_of_unity()1/2*a + 1/2

I get

1/2*a + 1/2

rather than the

-1/2*a + 1/2

in the doctest.

Now if edit the doctest to read

            sage: K = QuadraticField(-3, 'b')
            sage: K.primitive_root_of_unity()
            -1/2*b + 1/2
            sage: UK = K.unit_group()
            sage: K.primitive_root_of_unity()
            -1/2*b + 1/2

rebuild and test I get

Failed example:
    K.primitive_root_of_unity()
Expected:
    -1/2*b + 1/2
Got:
    1/2*b + 1/2

The reason that there is no failure when the generator has the default name 'a' is caused by the fact that the unit group of this field has already been computed in a doctest for the S_units method. Thus as written the doctest is not testing that the problem has been fixed, since both instances of K.primitive_root_of_unity() use the same code.

comment:9 Changed 12 months ago by cremona

Wow, that is subtlety i had certainly missed. I cannot look into it now as I'm about to go on holiday, so feel free to repatch, or wait.

comment:10 Changed 12 months ago by fwclarke

Enjoy your holiday. I too am off on holiday, so I'll wait.

comment:11 Changed 12 months ago by jdemeyer

Since the primitive root of unity is computed using PARI in both cases (either as part of full unit group or using nfrootsof1), wouldn't the problem be solved by simply returning whatever is answered by PARI? That is, don't try to find the generator with shortest string representation.

comment:12 Changed 10 months ago by jdemeyer

  • Authors changed from John Cremona to John Cremona, Jeroen Demeyer
  • Description modified (diff)
  • Status changed from needs_work to needs_review

Changed 10 months ago by jdemeyer

Apply to 5.11.rc0

comment:13 Changed 10 months ago by pbruin

  • Cc pbruin removed
  • Reviewers set to Peter Bruin
  • Status changed from needs_review to positive_review

Patch looks good and solves the problem. Doctests pass on x86_64 GNU/Linux. (I don't expect any 32-bit/64-bit differences, which can occur for some computations with number fields in PARI.)

comment:14 Changed 10 months ago by jdemeyer

  • Merged in set to sage-5.13.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.