Opened 9 years ago
Closed 9 years ago
#9266 closed defect (fixed)
bug in global_integral_model for Elliptic Curves over Number Fields
Reported by: | wuthrich | Owned by: | 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 9 years ago by
comment:2 follow-up: ↓ 3 Changed 9 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 in reply to: ↑ 2 Changed 9 years ago by
Replying to wuthrich:
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.
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 9 years ago by
That holds for me too :) We will do it in California !
See you soon.
comment:5 Changed 9 years ago by
- Milestone changed from sage-4.4.5 to sage-4.5
Milestone sage-4.4.5 deleted
comment:6 Changed 9 years ago by
- Status changed from new to needs_review
comment:7 Changed 9 years ago by
- Reviewers set to John Cremona
- Status changed from needs_review to positive_review
Looks good and tests pass on 4.4.4.alpha0
comment:8 Changed 9 years ago by
- Merged in set to sage-4.5.2.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
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?