Opened 11 years ago

Closed 11 years ago

# problem in evaluation dual isogeny

Reported by: Owned by: wuthrich cremona major sage-4.7.1 elliptic curves isogenies, sage-4.7.1.alpha0 Chris Wuthrich Luca De Feo, John Cremona N/A

### 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'>
```

### comment:1 Changed 11 years ago by wuthrich

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 ?

```sage: R.<x,y> = PolynomialRing(ZZ,2)
sage: f(x=2,y=0)
-2
sage: type(f(x=2,y=0))
<type 'sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular'>
```

It is certainly not consitent with this :

```sage: R.<x> = PolynomialRing(ZZ)
sage: f= x^2+3
sage: type(f(x=2))
<type 'sage.rings.integer.Integer'>
```

### comment:2 follow-up: ↓ 3 Changed 11 years ago by cremona

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 11 years ago by cremona

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.

See #8502.

### comment:4 Changed 11 years ago by wuthrich

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.

### Changed 11 years ago by wuthrich

exported against 4.6.2. Only this patch should be applied.

### comment:5 Changed 11 years ago by wuthrich

• Authors set to wuthrich
• 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 11 years ago by wuthrich

The other, more general issue is now #10946.

### comment:7 Changed 11 years ago by defeo

• 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 11 years ago by cremona

• 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 11 years ago by jdemeyer

• Milestone changed from sage-4.7 to sage-4.7.1

### comment:10 Changed 11 years ago by jdemeyer

• Authors changed from wuthrich to Chris Wuthrich

### comment:11 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.