Opened 12 years ago
Closed 11 years ago
#10677 closed enhancement (fixed)
Improve PARI interface for relative number fields
Reported by: | jdemeyer | Owned by: | davidloeffler |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7 |
Component: | number fields | Keywords: | pari rnf |
Cc: | mstreng, jdemeyer | Merged in: | sage-4.7.alpha1 |
Authors: | Jeroen Demeyer | Reviewers: | Marco Streng |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Attachments (2)
Change History (18)
comment:1 Changed 12 years ago by
- Status changed from new to needs_review
comment:2 Changed 12 years ago by
- Status changed from needs_review to needs_work
comment:3 Changed 12 years ago by
- Status changed from needs_work to needs_review
comment:4 follow-up: ↓ 5 Changed 12 years ago by
Looks good at first glance. Could you add doctests for get_nf(self)
on line 671 of sage/libs/pari/gen.pyx?
comment:5 in reply to: ↑ 4 Changed 12 years ago by
- Cc mstreng jdemeyer added
Replying to mstreng:
Could you add doctests for
get_nf(self)
?
(these would be implicit doctests I suppose)
Related to this: the documentation of get_nf
seems to be more detailed than that of nf_get_pol
. In particular, the documentation of nf_get_pol
doesn't say that only nfinit, bnfinit and bnrinit are allowed (which I think it should). A doctest showing the error message for an rnfinit object would be useful.
I think doctests showing that these functions work for bnfinit and rnfinit would be good too.
comment:6 follow-up: ↓ 7 Changed 12 years ago by
- Status changed from needs_review to needs_work
Hi Jeroen,
Did you ever test your new code for converting PARI relative number field elements to Sage relative number field elements?
I get this (on 4.6.2.alpha3 regardless of whether I apply your patch):
sage: p = pari("Mod(Mod(x, x^2-y), y^3+17)"); p 0
Clearly "0" is incorrect. This seems to be a bug introduced by the PARI upgrade:
GP/PARI CALCULATOR Version 2.4.3 (alpha) (13:18) gp > Mod(Mod(x, x^2-y), y^3+17) %1 = 0 (13:19) gp > Mod(Mod(x, x^2-y), y^3+17)==0 %2 = 1
GP/PARI CALCULATOR Version 2.3.4 (released) (13:16) gp > Mod(Mod(x, x^2-y), y^3+17) %1 = Mod(Mod(x, x^2 - y), y^3 + 17) (13:20) gp > Mod(Mod(x, x^2-y), y^3+17)==0 %2 = 0
I can't test your patch this way:
sage: y = PolynomialRing(QQ,'y').gen() sage: K.<a> = NumberField(y^3+17) sage: x = PolynomialRing(K,'x').gen() sage: L.<b> = NumberField(x^2-a) sage: p = pari("Mod(Mod(x, x^2-y), y^3+17)") sage: L(p) 0 sage: p.sage() 0 sage: p.sage().parent() Integer Ring
It is possible that I am using PARI in the wrong way here. Regardless of that, your patch should contain an example of converting PARI relative number field elements to Sage relative number field elements.
comment:7 in reply to: ↑ 6 Changed 12 years ago by
Replying to mstreng:
Clearly "0" is incorrect.
Whether or not "0" is correct, I believe you are doing things the wrong way. The following is a proper example of a PARI relative number field element::
Mod(Mod(y, y^3 + 17)*x + Mod(1, y^3 + 17), x^2 - y)
Here, K is the field over QQ defined by y^3 + 17
and L is the extension defined by x^2 - y
.
There is an example in my patch in sage/rings/number_field/number_field_element.pyx
, line 3316. But it might be a good idea to expand this.
comment:8 Changed 12 years ago by
sage: sage: y = PolynomialRing(QQ,'y').gen() sage: sage: K.<a> = NumberField(y^3+17) sage: sage: x = PolynomialRing(K,'x').gen() sage: sage: L.<b> = NumberField(x^2-a) sage: p = pari("Mod(Mod(y, y^3 + 17)*x + Mod(1, y^3 + 17), x^2 - y)") sage: L(p) a*b + 1
comment:9 Changed 12 years ago by
- Status changed from needs_work to needs_review
Added another example.
Changed 12 years ago by
comment:10 follow-up: ↓ 11 Changed 12 years ago by
Replying to jdemeyer:
The following is a proper example of a PARI relative number field element::
Mod(Mod(y, y^3 + 17)*x + Mod(1, y^3 + 17), x^2 - y)
Of course, sorry about that.
I don't know how much work it is, but as you are adding extra examples anyway, perhaps you could fix this:
sage: K.<a> = QuadraticField(-5) sage: p = pari("Mod(x,x^2+7)") sage: K(p) a sage: K(p)^2 -5 sage: p^2 Mod(-7, x^2 + 7)
The same issue is there with your code:
sage: y = PolynomialRing(QQ,'y').gen() sage: K.<a> = NumberField(y^3+17) sage: x = PolynomialRing(K,'x').gen() sage: L.<b> = NumberField(x^2-a) sage: q = pari("Mod(Mod(y, y^3 + 19)*x + Mod(1, y^3 + 23), x^2 - y + 1)") sage: L(q) a*b
I think K(p) and L(q) should raise errors.
There is an example in my patch in
sage/rings/number_field/number_field_element.pyx
, line 3316. But it might be a good idea to expand this.
Great! I'd say
- at least one example with the completely written out correct form of PARI, in case idiots like me swap the moduli around again.
- at least one example with the incorrect moduli that raises an error (like my examples in this comment).
- at least one example in the documentation of the
__init__
method ofNumberFieldElement
(because that is where your new code is).
comment:11 in reply to: ↑ 10 Changed 12 years ago by
- Reviewers set to Marco Streng
Replying to mstreng:
- at least one example with the incorrect moduli that raises an error (like my examples in this comment).
Done for absolute elements. For relative elements, the checking is impossible because it should be done by PARI's rnfeltreltoabs()
, but it's not.
- at least one example in the documentation of the
__init__
method ofNumberFieldElement
(because that is where your new code is).
I decided not to do this, because it would lead to duplication and also because there isn't a single example of relative number fields in NumberFieldElement.__init__()
.
I've added the new patch, but I still need to doctest it (also together with #2329).
comment:12 Changed 12 years ago by
Great, I'll probably have some time to review it tomorrow.
comment:13 Changed 12 years ago by
All long doctests pass with this ticket and #2329 applied.
comment:15 Changed 12 years ago by
- Milestone changed from sage-4.6.2 to sage-4.7
comment:16 Changed 11 years ago by
- Merged in set to sage-4.7.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
I still need to implement conversion from a PARI relative number field element.