Clean up weierstrass_p

[jdemeyer]

Fix the precision bound for the quadratic algorithm and clean up the code.

CC: @JohnCremona @pjbruin

Component: elliptic curves

Author: Jeroen Demeyer

Branch/Commit: 666714e

Reviewer: Peter Bruin

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

[jdemeyer]

Branch: u/jdemeyer/ticket/15855

[sagetrac-git]

Commit: 1e69265

[sagetrac-git]

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

1e69265

Clean up weierstrass_p function

[jdemeyer]

New commits:

1e69265

Clean up weierstrass_p function

[jdemeyer]

Author: Jeroen Demeyer

[pjbruin]

comment:5

An argument to be substituted for %s is missing in the following line:

raise ValueError("for computing the Weierstrass p-function via pari, the characteristic (%s) of the underlying field must be zero")

[sagetrac-git]

Changed commit from 1e69265 to 89726c5

[sagetrac-git]

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

89726c5

Fix exception formatting

[pjbruin]

comment:8

In the meantime, Karim Belabas fixed the elliptic functions in PARI for base fields of positive characteristic. Maybe we should just change the test for the characteristic in the case algorithm='pari' accordingly?

[pjbruin]

comment:9

I did some further cleaning up on a new branch u/pbruin/15855-weierstrass_p_cleanup. The test for p >= prec + 3 can also be done at the beginning, since p really arise in the denominators of the series, we are not just dividing by it as an intermediate step of the algorithm. With this patch, we don't insist on characteristic 0 when algorithm='pari', but neither do we use PARI by default. If you agree with the changes in this branch and it still good as a dependency of #15767, you can put it in the branch field.

We probably have to check if everything still works after the latest PARI fix, hence needs_info.

[jdemeyer]

comment:10

Alternatively, repurpose this ticket as a cleanup ticket and leave the algorithm choice as it was.

[jdemeyer]

comment:11

Replying to @jdemeyer:

Alternatively, repurpose this ticket as a cleanup ticket and leave the algorithm choice as it was.

Peter, let me know if this sounds like a good idea, then I will implement it.

[pjbruin]

comment:12

Replying to @jdemeyer:

Replying to @jdemeyer:

Alternatively, repurpose this ticket as a cleanup ticket and leave the algorithm choice as it was.

Peter, let me know if this sounds like a good idea, then I will implement it.

Yes, if PARI's ellwp() works as intended again, I think that is the best plan.

[sagetrac-git]

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

0e4fff5

Clean up weierstrass_p function

[sagetrac-git]

Changed commit from 89726c5 to 0e4fff5

[pjbruin]

Reviewer: Peter Bruin

[pjbruin]

comment:16

The last part of the new changelog entry in ell_wp.py is no longer accurate. Besides that, positive review.

(Maybe the additional change proposed in comment:9 isn't the best one; perhaps there are elliptic curves where more coefficients of the series are well defined, in which case a NotImplementedError is better than an algorithm-independent ValueError.)

[sagetrac-git]

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

666714e

Fix changelog entry for ell_wp

[sagetrac-git]

Changed commit from 0e4fff5 to 666714e

[jdemeyer]

comment:18

Replying to @pjbruin:

The last part of the new changelog entry in ell_wp.py is no longer accurate. Besides that, positive review.

Done.

(Maybe the additional change proposed in comment:9 isn't the best one; perhaps there are elliptic curves where more coefficients of the series are well defined, in which case a NotImplementedError is better than an algorithm-independent ValueError.)

I have no idea, I don't know the mathematics well enough.

[vbraun]

Changed branch from u/jdemeyer/ticket/15855 to 666714e