change sorting for WeierstrassIsomorphism

[yyyyx4]

Currently, EllipticCurve_generic.automorphisms() returns the automorphisms in a more or less arbitrary (albeit deterministic) order.

It is much more natural to users to receive a list with the identity and negation first, since they exist for any curve, then any other automorphisms that may exist. (I have personally seen code making this incorrect assumption.)

In this patch, we change ._comparison_impl() for WeierstrassIsomorphism in such a way that [1] and [-1] will appear first in a sorted list of automorphisms.

Diff without the dependencies: sagemath/sagetrac-mirror@d92c9f4...c9e9646

Depends on #34727
Depends on #34722

CC: @JohnCremona

Component: elliptic curves

Author: Lorenz Panny

Branch/Commit: c9e9646

Reviewer: John Cremona

Issue created by migration from https://trac.sagemath.org/ticket/34728

[JohnCremona]

Reviewer: John Cremona

[JohnCremona]

comment:2

All looks good, and I agree with the sorting strategy. Two questions: (1) when p=2 in the random test is it still the case that the second auto in the list is multiplcation by -1? (2) In the test for the length of the autos, for the last two cases you don't really need to put both 0 and 1728 in the list since they are equal :) But I will not insist.

As long as the answer to (1) is yes, you can take a positive review from me.

[sagetrac-git]

Changed commit from 6eaaf7e to c9e9646

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

4c4ce93	0 == 1728 in characteristics 2 and 3
c9e9646	compare entire (u,r,s,t) tuple

[yyyyx4]

comment:4

Thank you for the feedback!

(2) Done.

(1) The answer used to be no (I had incorrectly assumed that u uniquely identifies the isomorphism, which is manifestly untrue in characteristics 2 and 3), but it should be handled correctly now. This did require moving the comparison up from baseWI to WeierstrassIsomorphism, as the former knows nothing about the domain curve.

Since the new changes are not totally trivial, I'll let you have another look rather than setting it to positive review right away.

Another question: Did you check all modifications in this branch or just the ones in "diff without the dependencies"?

[JohnCremona]

comment:5

I checked all. Can't check again right now as travelling.

[JohnCremona]

comment:6

Thanks for the two extra changes. I like that the new sorting key code ends up several lines shorter!

[yyyyx4]

comment:7

Thank you! I'll also boldly go ahead and set #34722 to positive review, since the changes there are contained in this branch — which you've reviewed.

[JohnCremona]

comment:8

Replying to Lorenz Panny:

Thank you! I'll also boldly go ahead and set #34722 to positive review, since the changes there are contained in this branch — which you've reviewed.

OK fine

[vbraun]

Changed branch from public/sort_elliptic_curve_isomorphisms to c9e9646