Opened 13 months ago
Closed 10 months ago
#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)
Change History (15)
comment:1 Changed 13 months ago by cremona
comment:2 Changed 13 months ago by cremona
- Cc pbruin added
- Owner set to cremona
comment:3 follow-up: ↓ 4 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: ↓ 7 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
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
- Description modified (diff)
- Status changed from needs_work to needs_review
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
Patch fixes the problem, now testing.