Opened 12 years ago
Closed 10 years ago
#5618 closed defect (fixed)
Cyclotomic field elements are not converted to Gap correctly
Reported by: | saliola | Owned by: | tbd |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6.2 |
Component: | algebra | Keywords: | gap interface cyclotomic field |
Cc: | joyner | Merged in: | sage-4.6.2.alpha2 |
Authors: | Simon King | Reviewers: | Luis Felipe Tabera Alonso |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Although this works:
sage: K = CyclotomicField(3) sage: z = K.an_element(); z zeta3 sage: gap(z) zeta3
the resulting gap element doesn't have the correct properties:
sage: K(gap.E(3)) == z # Good! True sage: gap(K(gap.E(3))) == gap.E(3) # Bad! False
This causes the following problem with group characters.
sage: H = AlternatingGroup(4) sage: g = H.list()[1] sage: K = H.subgroup([g]) sage: z = CyclotomicField(3).an_element(); z sage: c = K.character([1,z,z**2]) ... RuntimeError: Gap produced error output Error, no 1st choice method found for `CONDUCTOR' on 1 arguments
Note: the above works if one takes z = gap.E(3).
Attachments (3)
Change History (12)
comment:1 Changed 11 years ago by
- Summary changed from Cyclotomic field elements are not convert to Gap correctly. to Cyclotomic field elements are not converted to Gap correctly
comment:2 Changed 10 years ago by
- Report Upstream set to N/A
comment:3 Changed 10 years ago by
- Keywords gap interface cyclotomic field added
- Status changed from new to needs_review
I created the patch after the patches from #8909 and #9423 -- so, it is possible that the new patch actually depends on the two other tickets (but one of them already has a positive review).
With the patch, a cyclotomic field in Sage is represented as a cyclotomic field (and not as a number field) in GAP:
sage: Z = CyclotomicField(8) sage: gap(Z) CF(8) sage: Z(gap(Z.0^2)) zeta8^2
The advantage is that the motivating example from the ticket description now works (and is used as a doctest):
sage: H = AlternatingGroup(4) sage: g = H.list()[1] sage: K = H.subgroup([g]) sage: z = CyclotomicField(3).an_element(); z sage: c = K.character([1,z,z**2]); c Character of Subgroup of AlternatingGroup(4) generated by [(2,3,4)] sage: c(g^2); z^2 -zeta3 - 1 -zeta3 - 1
The disadvantage: While it is still possible to chose the name of a number field generator in GAP as in Sage
sage: K.<tau> = NumberField(x^2+x+1) sage: gap(K) <algebraic extension over the Rationals of degree 3> sage: K.0 tau sage: gap(K.0) tau
it is now impossible to do the same for cyclotomic fields:
sage: L.<zeta> = CyclotomicField(3) sage: L.0 zeta sage: gap(L.0) E(3)
By consequence, I had to fix a couple of doctests. All doctests pass, but I can not vouch for external code.
comment:4 Changed 10 years ago by
- Reviewers set to Luis Felipe Tabera Alonso
- Status changed from needs_review to needs_work
The patch simply converts sage Cyclotomic fields to gap cyclotomic fields instead of generic number fields. The disadvantage presented is just a limitation of gap, not sage.
The semantics of gap(CyclotomicField?(n)) have changed! DeprecationWarning? would be too pedantic here. Current behaviour of sage is considered a bug and I cannot see any functionality loss. So the code is ok.
The doctests are relevant. However, this patch depends on #9423 and one doctest has disappeared in that patch. That doctest is relevant, because it shows the change in the code. So, I would add the output of:
sage: F=CyclotomicField(8) sage: F.gen() sage: F._gap_init_() # the following variable name $sage1 represents the F.base_ring() in gap and is somehow random sage: f=gap(F) sage: f.GeneratorsOfDivisionRing()
in NumberField_cyclotomic._gap_init_()
comment:5 Changed 10 years ago by
- Status changed from needs_work to needs_review
I give a positive review to Simon's patch. However, I have added some doctest in trac_5618_gap_for_cyclotomic_fields_doctest.patch that needs review. The doctest are in the unpatched code, but they have disappeared in the process. This patch put them back.
Apply:
trac_5618_gap_for_cyclotomic_fields.2.patch
trac_5618_gap_for_cyclotomic_fields_doctest.patch
Depends on: #9423
comment:6 Changed 10 years ago by
Should this be marked positive review then?
comment:7 Changed 10 years ago by
If you agreee with the doctest I wrote back in http://trac.sagemath.org/sage_trac/ticket/5618 yes, it should have a positive review.
comment:8 Changed 10 years ago by
- Status changed from needs_review to positive_review
The doctest looks fine to me. Let's get this in. (I'm not going to claim reviewer credit for this one -- I just verified that the doctest was the same as the one removed from #9423.)
comment:9 Changed 10 years ago by
- Merged in set to sage-4.6.2.alpha2
- Resolution set to fixed
- Status changed from positive_review to closed
Here is some problem analysis:
So, apparently GAP treats "the same" elements of a cyclotomic field differently if they have different names. But this isn't particularly surprising, since the two cyclotomic fields seem to have a totaly different representation in GAP:
Note that comparison of the two field in the GAP interface results in an error:
But GAP indeed considers the two fields to be different:
So, what does all this mean?
__cmp__
method of the GAP interface has a bug. An error raised by GAP when attempting "<" or ">" should be caught and then "=" should be tried._gap_init_
method for Sage cyclotomic fields (currently, it is inherited from number fields).It seems likely to me that after implementing 2., things will already work. But 1. should be fixed as well.