Opened 11 years ago

Closed 11 years ago

#11229 closed defect (fixed)

Rational maps attached to elliptic curve isogeny are incorrect

Reported by: johanbosman Owned by: cremona
Priority: major Milestone: sage-4.7.1
Component: elliptic curves Keywords: elliptic curves, isogeny
Cc: Merged in: sage-4.7.1.alpha0
Authors: Johan Bosman Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

sage: E = EllipticCurve([1,2,3,4,5])
sage: Eshort = E.short_weierstrass_model()
sage: phi = E.isogeny(E(0), Eshort)
sage: phiX, phiY = phi.rational_maps()
sage: phi(E(1,2))
(63 : 864 : 1)
sage: phiX(1,2), phiY(1,2)
(63, 7560)

Attachments (1)

trac_11229_isogeny_rational_function.patch (2.3 KB) - added by johanbosman 11 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 11 years ago by cremona

Maybe the case of degree 1 isogenies has not been tested properly.

You can use E.isomorphism_to(Eshort) instead in this case. The Weierstrass isomorphisms were implemented long before isogenies, and have a completely different type (though obviously one should be able to create a degree 1 isogeny from an isomorphism, and vice versa).

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

  • Authors set to Johan Bosman

Hi John,

The only reason why I used a degree 1 isogeny in the example was to keep the example simple; it goes wrong in more general cases as well.

But I've already found in the code where it goes wrong. I will upload a patch shortly.

Johan

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

Replying to johanbosman:

Hi John,

The only reason why I used a degree 1 isogeny in the example was to keep the example simple; it goes wrong in more general cases as well.

But I've already found in the code where it goes wrong. I will upload a patch shortly.

Excellent!

I think there was a similar bug recently reported for isogenies -- could you look for that to see if your patch corrects that too? I don't remember the number.

Johan

comment:4 follow-up: Changed 11 years ago by johanbosman

Do you mean #10888? That bug is unrelated to this one. ;).

comment:5 Changed 11 years ago by johanbosman

  • Status changed from new to needs_review

Changed 11 years ago by johanbosman

comment:6 Changed 11 years ago by johanbosman

Comment: the only difference between the two patches is the compulsory "Trac" line.

comment:7 in reply to: ↑ 4 Changed 11 years ago by cremona

Replying to johanbosman:

Do you mean #10888? That bug is unrelated to this one. ;).

Too bad.

I have no time now but will review at some point, unless someone else gets there first.

comment:8 Changed 11 years ago by cremona

  • Reviewers set to John Cremona
  • Status changed from needs_review to positive_review

The patch looks good to me, applies to 4.7.alpha5, and all tests in elliptic_curves pass.

comment:9 Changed 11 years ago by jdemeyer

  • Milestone changed from sage-4.7 to sage-4.7.1

comment:10 Changed 11 years ago by jdemeyer

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