Opened 11 years ago

Closed 11 years ago

#12151 closed defect (fixed)

Bug in global_integral_model for elliptic curves over number fields

Reported by: Johan Bosman Owned by: John Cremona
Priority: major Milestone: sage-5.0
Component: elliptic curves Keywords:
Cc: Merged in: sage-5.0.beta9
Authors: Johan Bosman, John Cremona Reviewers: David Loeffler
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

sage: K.<v> = NumberField(x^2 + 161*x - 150)
sage: E = EllipticCurve([25105/216*v - 3839/36, 634768555/7776*v - 98002625/1296, 634768555/7776*v - 98002625/1296, 0, 0])
sage: E.global_integral_model()
...
AssertionError: bug in global_integral_model: [-511235417/8*v + 238969025/4, 789861012140869185/32*v - 365842578320087625/16, -434331620876169353603835/32*v + 201170993209979865073875/16, 0, 0]

Attachments (1)

12151.patch (2.0 KB) - added by Johan Bosman 11 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 11 years ago by John Cremona

This can be fixed by changing

for P, _ in K.ideal(a.denominator()).factor():

on line 564 of ell_number_field.py to

for P in [P for P,e in K.ideal(a).factor() if e<0]:

or alternatively (I think)

for P, _ in a.denominator_ideal().factor():

I checked that the first alternative works.

NB I also think that the line

pi = K.uniformizer(P,'positive')

should be

pi = K.uniformizer(P,'negative')

since we will divide by a power of pi and want to make sure that the model stays integral at other primes. This does not matter in the example given where the class number is 1 so each pi will be an actual generator of the prime ideal.

Changed 11 years ago by Johan Bosman

Attachment: 12151.patch added

comment:2 Changed 11 years ago by Johan Bosman

Authors: Johan Bosman
Status: newneeds_review

Changing negative into positive was done in #7935, so I've decided to keep it positive. ;).

comment:3 in reply to:  2 Changed 11 years ago by John Cremona

Replying to johanbosman:

Changing negative into positive was done in #7935, so I've decided to keep it positive. ;).

I have CC'd Chris Wuthrich who made the patch at #7935 (where I made a comment on exactly that line).

comment:4 Changed 11 years ago by David Loeffler

Authors: Johan BosmanJohan Bosman, John Cremona
Reviewers: David Loeffler
Status: needs_reviewpositive_review

This looks fine to me.

comment:5 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-5.0.beta9
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.