Opened 10 years ago

Closed 10 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)

10677_pari_rnf.patch (26.4 KB) - added by jdemeyer 10 years ago.
10677_check.patch (3.2 KB) - added by jdemeyer 10 years ago.
Apply on top of previous patch

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I still need to implement conversion from a PARI relative number field element.

comment:3 Changed 10 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:4 follow-up: Changed 10 years ago by mstreng

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 10 years ago by mstreng

  • 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: Changed 10 years ago by mstreng

  • 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 10 years ago by jdemeyer

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 10 years ago by jdemeyer

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 10 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Added another example.

Changed 10 years ago by jdemeyer

comment:10 follow-up: Changed 10 years ago by mstreng

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 of NumberFieldElement (because that is where your new code is).

Changed 10 years ago by jdemeyer

Apply on top of previous patch

comment:11 in reply to: ↑ 10 Changed 10 years ago by jdemeyer

  • 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 of NumberFieldElement (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 10 years ago by mstreng

Great, I'll probably have some time to review it tomorrow.

comment:13 Changed 10 years ago by jdemeyer

All long doctests pass with this ticket and #2329 applied.

comment:14 Changed 10 years ago by mstreng

  • Status changed from needs_review to positive_review

Looks good.

comment:15 Changed 10 years ago by jdemeyer

  • Milestone changed from sage-4.6.2 to sage-4.7

comment:16 Changed 10 years ago by jdemeyer

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