test whether point is actually on the curve when evaluating elliptic-curve isomorphism

[yyyyx4]

In Sage 10.0:

sage: E = EllipticCurve(GF(101), [1,1])
sage: f = E.automorphisms()[0]
sage: EE = EllipticCurve(GF(101), [5,5])
sage: P = EE.lift_x(2)
sage: P in f.domain()
False
sage: f(P)
(2 : 15 : 1)
sage: f(P) in f.codomain()
True
sage: f.codomain().defining_polynomial()(*f(P))
12

Sage will happily "evaluate" a WeierstrassIsomorphism on just about any EllipticCurvePoint, even a point which explicitly lies on a different curve. This simple patch adds a check to remove this footgun.

[kwankyu]

How about the case that $P$ is a point in the ambient plane of the elliptic curve and happens to be on the curve? Would $f(P)$ work with your patch? I think it should.

Currently it does

sage: E.ambient_space()
Projective Space of dimension 2 over Finite Field of size 101
sage: E.random_point()
(93 : 84 : 1)
sage: P = E.ambient_space()
sage: P(93,84,1)
(93 : 84 : 1)
sage: p = P(93,84,1)
sage: p
(93 : 84 : 1)
sage: f(p)
(93 : 84 : 1)

[JohnCremona]

The patch should add a doctest to illustrate the error now raised.

@kwankyu I'm not sure about that. If the argument P supplied was not an elliptic curve point but only a point in P2, would you raise an error if P is not on a curve? Would you regard the Weierstrass map as a map from P2 to itself and return another point in P2 (whether or not P or f(P) are on the curve)? Or if you did test whether P is on the curve would the returned pint be a point on the curve even though P was not?

This looks like a can of worms to me, and checking that P is in the actual domain in the strong sense, as in this patch, seems the simplest way. Let's merge this (once it has been rebased) and if you want to open an issue then do so.

[JohnCremona]

I'm adding "needs work" just to get the doctest added. Then I would give this a positive review, but should allow @kwankyu to respond to my comments above first.

[kwankyu]

@kwankyu I'm not sure about that. If the argument P supplied was not an elliptic curve point but only a point in P2, would you raise an error if P is not on a curve? Would you regard the Weierstrass map as a map from P2 to itself and return another point in P2 (whether or not P or f(P) are on the curve)? Or if you did test whether P is on the curve would the returned pint be a point on the curve even though P was not?

If f is a map from a general curve in P2, then I would expect that f(P) gives a point in the codomain of the map after coercing P to a point on the curve (and raise an error if not coercible).

This looks like a can of worms to me, and checking that P is in the actual domain in the strong sense, as in this patch, seems the simplest way. Let's merge this (once it has been rebased) and if you want to open an issue then do so.

Here we are dealing with elliptic curves and morphisms between them. Things should work as expected by experts in this domain of research. So I do not object to the patch if it is "right" for you.

[kwankyu]

By the way, I am just thankful to you for reviewing PRs. I think we are short of domain expert reviewers, especially on elliptic curves.

[yyyyx4]

Apparently EllipticCurveIsogeny does convert points in ℙ² to its domain (and throws an error only when this fails, i.e., when the point really doesn't lie on the curve). I don't have a lot of strong opinions on this matter, except that (1) behaviour should be consistent between EllipticCurveHom children, and (2) the current behaviour of WeierstrassIsomorphism increases the risk of mistakes and is therefore undesirable.

[JohnCremona]

By the way, I am just thankful to you for reviewing PRs. I think we are short of domain expert reviewers, especially on elliptic curves.

Thanks, I have been one of the major contributors to the elliptic curves section of Sage since the very beginning, and still have more that I want to do. I should have plenty of time to review others' hard work, being retired!

[JohnCremona]

Apparently EllipticCurveIsogeny does convert points in ℙ² to its domain (and throws an error only when this fails, i.e., when the point really doesn't lie on the curve). I don't have a lot of strong opinions on this matter, except that (1) behaviour should be consistent between EllipticCurveHom children, and (2) the current behaviour of WeierstrassIsomorphism increases the risk of mistakes and is therefore undesirable.

Agreed re consistency! Regarding @kwankyu 's suggestion, I prefer to leave it as it as (+ this patch), leaving more general questions about morphisms in algebraic geometry to others. There's a good reason (efficiency via special-casing) for using custom code for elliptic curve morphisms.

[The very first contribution I made to Sage in 2007 (apart from rewriting eclib to make it possible to interface with it) was to implement Weierstrass morphisms. I did not know much python back then (and even now see plenty of pythonisms I didn't know existed, especially when reviewing @yyyyx4 's code :)]

[yyyyx4]

Agreed re consistency!

In the newest version of this branch I've changed .__call__() to ._call_() so that everything works the same as for the other EllipticCurveHoms, including the check that the input point lies on the domain. Perhaps it's still debatable what the correct behaviour should be, but at least it's equally debatable for all ellliptic-curve morphisms with this fix.

[I did not know much python back then (and even now see plenty of pythonisms I didn't know existed, especially when reviewing @yyyyx4 's code :)]

To be fair, a lot of modern-day Pythonisms didn't even exist 15 years back... 🙂

[github-actions]

Documentation preview for this PR (built with commit 94068f7; changes) is ready! 🎉