Opened 12 years ago

Closed 12 years ago

## #9266 closed defect (fixed)

# bug in global_integral_model for Elliptic Curves over Number Fields

Reported by: | wuthrich | Owned by: | John Cremona |
---|---|---|---|

Priority: | minor | Milestone: | sage-4.5.2 |

Component: | elliptic curves | Keywords: | |

Cc: | Merged in: | sage-4.5.2.alpha0 | |

Authors: | Chris Wuthrich | Reviewers: | John Cremona |

Report Upstream: | N/A | Work issues: | |

Branch: | Commit: | ||

Dependencies: | Stopgaps: |

### Description

The following illustrates the bug. It should be easy to fix.

sage: K.<s> = NumberField(x^2-5) sage: w = (1+s)/2 sage: E = EllipticCurve(K,[2,w]) sage: E.global_integral_model() ...sage/schemes/elliptic_curves/ell_number_field.pyc in global_integral_model(self) 377 ai = [ai[i]/pi**(e*[1,2,3,4,6][i]) for i in range(5)] 378 for z in ai: --> 379 assert z.denominator() == 1, "bug in global_integral_model: %s" % ai 380 return EllipticCurve(list(ai)) 381 TypeError: not all arguments converted during string formatting

So there are two problems. One that the string is not correctly formatted, the other that it is raised. The latter, I believe, is just because the wrong thing is tested:

sage: w.denominator() 2 sage: w.is_integral() True

### Attachments (1)

### Change History (9)

### comment:1 Changed 12 years ago by

### comment:2 follow-up: 3 Changed 12 years ago by

*w* is the algebraic integer (1+sqrt(5))/2 and it is the coefficient of this integral Weierstrass equation. So this is a global_integral_model. We should not check if the denominator in some basis is 1, but rather if the coefficients are integers.

### comment:3 Changed 12 years ago by

Replying to wuthrich:

wis the algebraic integer (1+sqrt(5))/2 and it is the coefficient of this integral Weierstrass equation. So this is a global_integral_model. We should not check if the denominator in some basis is 1, but rather if the coefficients are integers.

OK then so we should do

if not all([z.is_integral() for z in ai]):

I'm too busy writing lectures for SD22 to make the patch myself!

### comment:4 Changed 12 years ago by

That holds for me too :) We will do it in California !

See you soon.

### comment:6 Changed 12 years ago by

Authors: | → Chris Wuthrich |
---|---|

Status: | new → needs_review |

### comment:7 Changed 12 years ago by

Reviewers: | → John Cremona |
---|---|

Status: | needs_review → positive_review |

Looks good and tests pass on 4.4.4.alpha0

### comment:8 Changed 12 years ago by

Merged in: | → sage-4.5.2.alpha0 |
---|---|

Resolution: | → fixed |

Status: | positive_review → closed |

**Note:**See TracTickets for help on using tickets.

The test would be better written as

The problem with the string is that it worked when ai was a list, but now it's a tuple.

I don't understand the last part -- what is w here?