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: |
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)
Change History (11)
comment:1 Changed 11 years ago by
comment:2 follow-up: ↓ 3 Changed 11 years ago by
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
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: ↓ 7 Changed 11 years ago by
Do you mean #10888? That bug is unrelated to this one. ;).
comment:5 Changed 11 years ago by
- Status changed from new to needs_review
Changed 11 years ago by
comment:6 Changed 11 years ago by
Comment: the only difference between the two patches is the compulsory "Trac" line.
comment:7 in reply to: ↑ 4 Changed 11 years ago by
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
- 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
- Milestone changed from sage-4.7 to sage-4.7.1
comment:10 Changed 11 years ago by
- Merged in set to sage-4.7.1.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
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).