Bug in global_integral_model for elliptic curves over number fields

[johanbosman]

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]

[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.

[johanbosman]

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

[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).

[davidloeffler]

This looks fine to me.