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)

trac_9266.patch (1.6 KB) - added by wuthrich 9 years ago.
exported against 4.4.4.alpha0

Download all attachments as: .zip

Change History (9)

comment:1 Changed 9 years ago by cremona

The test would be better written as

if not all([z.denominator()==1 for z in ai]):
    raise error

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?

comment:2 follow-up: Changed 9 years ago by 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.

comment:3 in reply to: ↑ 2 Changed 9 years ago by cremona

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 wuthrich

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

See you soon.

comment:5 Changed 9 years ago by was

  • Milestone changed from sage-4.4.5 to sage-4.5

Milestone sage-4.4.5 deleted

Changed 9 years ago by wuthrich

exported against 4.4.4.alpha0

comment:6 Changed 9 years ago by wuthrich

  • Authors set to Chris Wuthrich
  • Status changed from new to needs_review

comment:7 Changed 9 years ago by cremona

  • 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 mpatel

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