make PARI the default for EllipticCurve_field.weierstrass_p()

[yyyyx4]

On 9.5.rc3:

sage: E = EllipticCurve(GF(65537), [1, 2, 3, 4, 5])
sage: %timeit E.weierstrass_p(prec=1000)
229 ms ± 1.81 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
sage: %timeit E.weierstrass_p(prec=1000, algorithm='pari')
52.1 ms ± 1.15 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

PARI is significantly faster in all examples I've tried. We should use it by default instead of Sage's own implementation.

Depends on #33224

CC: @JohnCremona @defeo @videlec

Component: elliptic curves

Author: Lorenz Panny

Branch/Commit: fbffbf3

Reviewer: Sébastien Labbé, John Cremona, Samuel Lelièvre

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

[yyyyx4]

Dependencies: #33224

[yyyyx4]

Author: Lorenz Panny

[yyyyx4]

New commits:

6b2cd18

use PARI's ellwp() by default (it's faster than the "fast" implementation)

[yyyyx4]

Commit: 6b2cd18

[yyyyx4]

Branch: public/use_pari_ellwp_by_default

[seblabbe]

comment:5

I confirm the new default is better:

sage: %timeit E.weierstrass_p(prec=1000, algorithm='fast')
268 ms ± 580 µs per loop (mean ± std. dev. of 7 runs, 1 loop each)
sage: %timeit E.weierstrass_p(prec=1000, algorithm='pari')
58.3 ms ± 160 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: %timeit E.weierstrass_p(prec=1000, algorithm='quadratic')
86.4 ms ± 167 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: %timeit E.weierstrass_p(prec=1000)
57.7 ms ± 197 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

While reviewing, I noticed typos in the documentation:

sage: E.weierstrass_p??

I suggest to fix as follows:

-        - ``mprec`` - precision
+        - ``prec`` - precision

-        - ``algorithm`` - string (default:``None``) an algorithm identifier
-                      indicating using the ``pari``, ``fast`` or ``quadratic``
-                      algorithm. If the algorithm is ``None``, then this
-                      function determines the best algorithm to use.
+        - ``algorithm`` - string (default: ``None``) an algorithm identifier
+          indicating using the ``pari``, ``fast`` or ``quadratic``
+          algorithm. If the algorithm is ``None``, then this
+          function determines the best algorithm to use.

[seblabbe]

Reviewer: Sébastien Labbé

[sagetrac-git]

Changed commit from 6b2cd18 to 8daecaf

[sagetrac-git]

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

8daecaf

doc tweaks

[yyyyx4]

comment:7

Thanks! Updated documentation.

[seblabbe]

comment:8

I checked the changes. Looks good. Only one thing.

I don't agree with the following change:

-        - ``algorithm`` - string (default:``None``) an algorithm identifier
-                      indicating using the ``pari``, ``fast`` or ``quadratic``
-                      algorithm. If the algorithm is ``None``, then this
-                      function determines the best algorithm to use.
+        - ``algorithm`` -- an algorithm name or (default) ``None``:
+          See :func:`sage.schemes.elliptic_curves.ell_wp.weierstrass_p`
+          for a list of possible values.

Because for any user that get there, it is force one more step to get the possible values which is very often a question that comes to mind. Also, for normal user, they might just not be able to get to this other method.

In summary, I think it is better to leave it with an explicit list even if the information is doubled.

[JohnCremona]

comment:9

I agree with slabbe's comment. (I hate it when a function has a *kwds argument and the docstring does not explain them but sends you some where else, which is similar.)

[sagetrac-git]

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

741f1db

un-deduplicate documentation per comments in #33223

[sagetrac-git]

Changed commit from 8daecaf to 741f1db

[yyyyx4]

comment:11

Alright, makes sense. I changed it back (and added a comment to ell_wp.py to remind people of the duplication).

[seblabbe]

comment:12

Thanks for the changes.

I still have some comments with respect to coding conventions in sage with respect to documentation. Sorry if it is troublesome: it is just some details.

The following change

-        - ``algorithm`` - string (default:``None``) an algorithm identifier
+        - ``algorithm`` -- string or (default) ``None``: an algorithm

does not respect the coding convention for the documentation. See https://doc.sagemath.org/html/en/developer/coding_basics.html To be more precise, please keep string (default:``None``) as is (twice).

Also, it would more exact to write
``'pari'``
``'fast'``
``'quadratic'``
as opposed to
``pari``
``fast``
``quadratic``. Observe that ``None`` is okay, since it is not about the string ``'None'``.

[sagetrac-git]

Changed commit from 741f1db to 07045c2

[sagetrac-git]

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

07045c2

follow doc formatting convention

[yyyyx4]

comment:14

The reason I'd changed it was precisely that None is not a string, so saying that this parameter is "a string which defaults to None" is not type-correct. Realistically speaking it's clear enough either way.

[slel]

comment:15

How about:

        - ``algorithm`` -- string or ``None`` (default: ``None``):
          a choice of algorithm among ``"pari"``, ``"fast"``
          ``"quadratic"``; or ``None`` to let this function
          determine the best algorithm to use.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bf799ad	use PARI's ellwp() by default (it's faster than the "fast" implementation)
fbffbf3	doc tweaks

[sagetrac-git]

Changed commit from 07045c2 to fbffbf3

[yyyyx4]

comment:17

Done, and squashed all the documentation edits to untangle the commit history a bit.

[slel]

Changed reviewer from Sébastien Labbé to Sébastien Labbé, John Cremona, Samuel Lelièvre

[slel]

comment:18

Looks good to me if other reviewers agree.

[JohnCremona]

comment:19

Fine by me. The moral of this story is never to label an algorithm as 'fast'.

[seblabbe]

comment:20

Replying to @JohnCremona:

Fine by me. The moral of this story is never to label an algorithm as 'fast'.

Indeed!

[seblabbe]

comment:21

I confirm that make works including the doc:

[sagemath_doc_html-none] Build finished.

and All tests passed! in folder sage -tp src/sage/schemes/elliptic_curves.

[yyyyx4]

comment:22

Thanks everyone!

[vbraun]

Changed branch from public/use_pari_ellwp_by_default to fbffbf3