Opened 7 years ago
Closed 7 years ago
#14472 closed defect (fixed)
some elliptic curve functions over number fields fail over relative fields
Reported by: | cremona | Owned by: | cremona |
---|---|---|---|
Priority: | major | Milestone: | sage-5.10 |
Component: | elliptic curves | Keywords: | elliptic curve relative number field |
Cc: | alejandroargaez@… | Merged in: | sage-5.10.beta1 |
Authors: | John Cremona, Jeroen Demeyer | Reviewers: | Jeroen Demeyer, John Cremona |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
This was reported by Alejandro Argaez:
sage: K1.<w>=NumberField(x^2+x+1) sage: m=polygen(K1) sage: K2.<v>=K1.extension(m^2-w+1) sage: E=EllipticCurve([0*v,-432]) sage: E.global_minimal_model() <boom>
The error is that the degree() function is called on the ring of integers of a relative number field.
In fixing this bug (which should be easy) it would be a good idea to add relative examples to as many functions as possible in ell_numberfield.py
Attachments (2)
Change History (17)
comment:1 follow-up: ↓ 2 Changed 7 years ago by
comment:2 in reply to: ↑ 1 Changed 7 years ago by
Replying to cremona:
Note: after changing ZK.degree() to ZK.absolute_degree() on line 651 of
ell_numberfield.py
it still fails, since the code in lines 656--658 (which I wrote, I think) does not work for relative number fields. The purpose of these lines is to set r,s,t to "least residues" modulo 2,3,2 of three successive quantities.
Follow-up: the old code for _reduce_model() was flawed, as follows: to reduce a1,a2,a3 modulo 2,3,2 respectively, it attempted to reduce their coordinates as given by list(a1), etc. Firstly this fails for relative extensions, but it is also misguided since there is no reason why the list() coordinates should be integral. I have changed this to work with the coordinates with respect to an integral basis, which is good for relative extensions. Only one small problem: the doctest on line 860 which used to return (as a minimal model over Q(a) where a=sqrt(5))
(0, 1, 0, a - 33, -2*a + 64)
but now gives
(0, -3/2*a - 1/2, 0, 3/2*a - 59/2, 27/2*a + 155/2)
which does not look so nice. Note that the integral basis here is [1/2*a + 1/2, a] and that with respect to this basis 1 has coordinates (2,-1) while -3/2*a - 1/2 has coordinates (-1,-1) so (counterintuitively) 1 is not reduced mod 2 but -(3a+1)/2 is!
Note that the integral basis computed does depend on the polynomial used to generate the field:
sage: QuadraticField(5,'a').ring_of_integers().gens() [1/2*a + 1/2, a] sage: NumberField(x^2-x-1,'a').ring_of_integers().gens() [1, a]
comment:3 Changed 7 years ago by
- Status changed from new to needs_review
comment:4 follow-up: ↓ 5 Changed 7 years ago by
All the examples seem to be quadratic number fields, is this intentional?
I don't know why Sage returns the basis of ZK
like that, because it's not what PARI gives:
sage: K.<a> = NumberField(x^2-5) sage: K.integral_basis() [1/2*a + 1/2, a] sage: K._pari_integral_basis() [1, 1/2*y - 1/2]
As for reducing an element modulo an ideal (which is what you do here), you could use PARI's nfeltreduce()
.
comment:5 in reply to: ↑ 4 Changed 7 years ago by
Replying to jdemeyer:
All the examples seem to be quadratic number fields, is this intentional?
No, probably just laziness.
I don't know why Sage returns the basis of
ZK
like that, because it's not what PARI gives:sage: K.<a> = NumberField(x^2-5) sage: K.integral_basis() [1/2*a + 1/2, a] sage: K._pari_integral_basis() [1, 1/2*y - 1/2]
Well spotted. The integral_basis method calls maximal_order which does call _pari_integral_basis, but then applies some Order constructor to the generators (order.absolute_order_from_module_generators) which is where this non-canonical ( to my mind) basis comes from. If that is to be chaned for quadratic fields then that would be a separate ticket, and would surely have a lot of doctest output consequences.
As for reducing an element modulo an ideal (which is what you do here), you could use PARI's
nfeltreduce()
.
Sure, but here we are only reducing modulo (2) or (3) so it seemed easier to do it manually.
comment:6 Changed 7 years ago by
- Description modified (diff)
- Reviewers set to Jeroen Demeyer
I made a new patch using PARI's nfeltdiveuc
. This gives simpler code and has the advantage that 1 is reduced, so there is no need to change the field.
comment:7 follow-up: ↓ 8 Changed 7 years ago by
- Status changed from needs_review to needs_work
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 7 years ago by
Replying to jdemeyer: I like the use of nfeltdiveuc, but (as you have maybe already noticed) the values of r,s,t are not the reduced values....so I'll wait for the next version of your patch!
comment:9 in reply to: ↑ 8 Changed 7 years ago by
comment:10 follow-up: ↓ 12 Changed 7 years ago by
Sorry, I think I misunderstood the output of nfeltdiveuc. I see now that it gives the quotient, not the remainder, and that is correct. So it is good. I am testing -- so why does it need work?
Also, I am getting an warning that line 9821 in sage.libs.pari.gen.pyx is unreachable code. Does tat need looking into?
comment:11 Changed 7 years ago by
- Status changed from needs_work to needs_review
Changed 7 years ago by
comment:12 in reply to: ↑ 10 Changed 7 years ago by
Replying to cremona:
Also, I am getting an warning that line 9821 in sage.libs.pari.gen.pyx is unreachable code. Does tat need looking into?
That has nothing to do with this ticket, but I fixed it anyway.
comment:13 Changed 7 years ago by
John, do you want to formally review my patch? I give positive review to the parts which were copied from your initial patch.
comment:14 Changed 7 years ago by
- Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, John Cremona
- Status changed from needs_review to positive_review
I give positive review to the parts which Jeroen added, hance we have an overall positive review.
comment:15 Changed 7 years ago by
- Merged in set to sage-5.10.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
Note: after changing ZK.degree() to ZK.absolute_degree() on line 651 of
ell_numberfield.py
it still fails, since the code in lines 656--658 (which I wrote, I think) does not work for relative number fields. The purpose of these lines is to set r,s,t to "least residues" modulo 2,3,2 of three successive quantities.