Opened 8 years ago
Closed 8 years ago
#10888 closed defect (fixed)
problem in evaluation dual isogeny
Reported by: | wuthrich | Owned by: | cremona |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.1 |
Component: | elliptic curves | Keywords: | isogenies, |
Cc: | Merged in: | sage-4.7.1.alpha0 | |
Authors: | Chris Wuthrich | Reviewers: | Luca De Feo, John Cremona |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
The following bug was reported by J. Gillibert.
sage: K.<th> = NumberField(x^2+3) sage: E = EllipticCurve(K,[7,0]) sage: phi = E.isogeny(E(0,0)) sage: P = E(-3,4*th) sage: phi(P) (-16/3 : 8/9*th : 1) sage: Q = phi(P) sage: phihat = phi.dual() sage: phihat(Q)
This gives back a
TypeError: <type 'sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular'>
Attachments (2)
Change History (13)
comment:1 Changed 8 years ago by
comment:2 follow-up: ↓ 3 Changed 8 years ago by
I came across something very similar indeed before and thought I had fixed it. I should check to see if the patch I made ever got merged.
comment:3 in reply to: ↑ 2 Changed 8 years ago by
comment:4 Changed 8 years ago by
John, thanks to your link I found quickly what is the problem. Consider
f(2,3) f(x=2,y=3) f.subs(x=2,y=3)
The first returns an element in the base ring, while the others are still polynomials. This is correct with respect to the documentation apart from the middle one. f(x=2,y=3)
is the __call__
method and there, in the first lines, it jumps to execute subs
.
So something has to be changed there. I am uncertain what. I attach a patch that changes __call__
to make it coherent with the documentation, but it is not ready for review because it will most certainly produce many problems in other places...
I will aks this to sage-devel.
comment:5 Changed 8 years ago by
- Milestone set to sage-4.7
- Status changed from new to needs_review
From the discussion on sage-devel, I learned that I should not change the functioning of evaluation in this ticket. To solve the bug in the isogenies, I simply make the evaluations in the code to be evaluations without keywords. Then it is no longer ambiguous.
I will open another ticket about the issue with evaluation and substitution.
comment:6 Changed 8 years ago by
The other, more general issue is now #10946.
comment:7 Changed 8 years ago by
- Cc defeo added
- Status changed from needs_review to needs_work
The patch works and I'm willing to give a positive review, but the doctest takes 2.6 cpu secs on my laptop (64-bit Intel Core2 U9400 @ 1.40GHz... not the fastest cpu ever, but still decent), it would make sense to tag this test as long.
Alternatively, though I don't know if this is acceptable for Sage standards, the doctest could be slightly changed. The code below fails in 4.6.2 and is fixed by this patch; it takes only 0.2 secs, because it uses Kohel's algorithm (instead of Vélu's formulae) to compute phi
and because avoids the call to phi.dual()
(which uses Stark's algorithm). It tests exactly the same bug, because it crosses the same conditional branches modified by the patch.
sage: K.<th> = NumberField(x^2+3) sage: E = EllipticCurve(K,[7,0]) sage: _.<X> = K[] sage: phi = E.isogeny(X) sage: phi.set_pre_isomorphism(E.automorphisms()[1]) sage: phi.set_post_isomorphism(phi.codomain().automorphisms()[1]) sage: P = E(-3,4*th) sage: phi(P) (-16/3 : 8/9*th : 1)
Finally, the doctest does not show up in the reference manual, because it is in the docstring of the method __call__
. I don't know if this really is a problem.
comment:8 Changed 8 years ago by
- Cc defeo removed
- Reviewers set to Luca De Feo, John Cremona
- Status changed from needs_work to positive_review
I have also tested the patch and agree that it works (applied to 4.7.alpha5, all tests in elliptic curves pass).
I don't think that the test is too long. This whole file is quite long as a whole (nearly 30s) but several other tests are longer! I expect to be splitting this file into two before long, so I am happy to leave off the "long time" tag. I would also be happy to have defeo's additional code added.
It doesn't matter that this example does not appear in the reference manual. It stays in as a test, to make sure this will continue to work in future; but it does not add a lot to for the user, I think. If you want it to appear in the manual, insert it in the header section of the file (instead, or as well).
I'll set this to "positive review" now; if you (defeo) wants to make further changes to the tests put it back to "needs review" and I'll look at it again.
comment:9 Changed 8 years ago by
- Milestone changed from sage-4.7 to sage-4.7.1
comment:10 Changed 8 years ago by
comment:11 Changed 8 years ago by
- Merged in set to sage-4.7.1.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
The problem is very simply that evaluation in a polynomial with two variables does not give back a constant, but a constant polynomial. Is this a bug ?
It is certainly not consitent with this :