Opened 8 years ago

Closed 8 years ago

#12151 closed defect (fixed)

Bug in global_integral_model for elliptic curves over number fields

Reported by: johanbosman Owned by: 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:

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 johanbosman 8 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 8 years ago by 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 8 years ago by johanbosman

comment:2 follow-up: Changed 8 years ago by johanbosman

  • Authors set to Johan Bosman
  • Status changed from new to needs_review

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

comment:3 in reply to: ↑ 2 Changed 8 years ago by 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 8 years ago by davidloeffler

  • Authors changed from Johan Bosman to Johan Bosman, John Cremona
  • Reviewers set to David Loeffler
  • Status changed from needs_review to positive_review

This looks fine to me.

comment:5 Changed 8 years ago by jdemeyer

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