Opened 9 years ago

Closed 9 years ago

#15855 closed enhancement (fixed)

Clean up weierstrass_p

Reported by: Jeroen Demeyer Owned by:
Priority: major Milestone: sage-6.2
Component: elliptic curves Keywords:
Cc: John Cremona, Peter Bruin Merged in:
Authors: Jeroen Demeyer Reviewers: Peter Bruin
Report Upstream: N/A Work issues:
Branch: 666714e (Commits, GitHub, GitLab) Commit: 666714eb35225661954ce037892f40d866cc04c3
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

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

Change History (19)

comment:1 Changed 9 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 9 years ago by Jeroen Demeyer

Branch: u/jdemeyer/ticket/15855
Created: Feb 24, 2014, 12:39:24 PMFeb 24, 2014, 12:39:24 PM
Modified: Feb 24, 2014, 1:29:43 PMFeb 24, 2014, 1:29:43 PM

comment:3 Changed 9 years ago by git

Commit: 1e692657b19d5ae80cd420e6a0c801a66cec6644

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

1e69265Clean up weierstrass_p function

comment:4 Changed 9 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer
Status: newneeds_review

New commits:

1e69265Clean up weierstrass_p function

comment:5 Changed 9 years ago by Peter Bruin

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")

comment:6 Changed 9 years ago by git

Commit: 1e692657b19d5ae80cd420e6a0c801a66cec664489726c59cdf83f3cee87e56806a3a86d8938df2c

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

89726c5Fix exception formatting

comment:7 Changed 9 years ago by Peter Bruin

Status: needs_reviewpositive_review

comment:8 Changed 9 years ago by Peter Bruin

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?

comment:9 Changed 9 years ago by Peter Bruin

Description: modified (diff)
Status: positive_reviewneeds_info

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.

comment:10 Changed 9 years ago by Jeroen Demeyer

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

comment:11 in reply to:  10 ; Changed 9 years ago by Jeroen Demeyer

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.

comment:12 in reply to:  11 Changed 9 years ago by Peter Bruin

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.

comment:13 Changed 9 years ago by Jeroen Demeyer

Description: modified (diff)
Summary: Disable PARI algorithm for weierstrass_p in positive characteristicClean up weierstrass_p

comment:14 Changed 9 years ago by git

Commit: 89726c59cdf83f3cee87e56806a3a86d8938df2c0e4fff510181e974717899f802108be8877a9346

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

0e4fff5Clean up weierstrass_p function

comment:15 Changed 9 years ago by Jeroen Demeyer

Status: needs_infoneeds_review

comment:16 Changed 9 years ago by Peter Bruin

Reviewers: Peter Bruin

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.)

comment:17 Changed 9 years ago by git

Commit: 0e4fff510181e974717899f802108be8877a9346666714eb35225661954ce037892f40d866cc04c3

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

666714eFix changelog entry for ell_wp

comment:18 in reply to:  16 Changed 9 years ago by Jeroen Demeyer

Status: needs_reviewpositive_review

Replying to pbruin:

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.

comment:19 Changed 9 years ago by Volker Braun

Branch: u/jdemeyer/ticket/15855666714eb35225661954ce037892f40d866cc04c3
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.