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:  sage6.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: 
Description (last modified by )
Fix the precision bound for the quadratic algorithm and clean up the code.
Change History (19)
comment:1 Changed 9 years ago by
Description:  modified (diff) 

comment:2 Changed 9 years ago by
Branch:  → u/jdemeyer/ticket/15855 

Created:  Feb 24, 2014, 12:39:24 PM → Feb 24, 2014, 12:39:24 PM 
Modified:  Feb 24, 2014, 1:29:43 PM → Feb 24, 2014, 1:29:43 PM 
comment:3 Changed 9 years ago by
Commit:  → 1e692657b19d5ae80cd420e6a0c801a66cec6644 

comment:4 Changed 9 years ago by
Authors:  → Jeroen Demeyer 

Status:  new → needs_review 
New commits:
1e69265  Clean up weierstrass_p function

comment:5 Changed 9 years ago by
An argument to be substituted for %s
is missing in the following line:
raise ValueError("for computing the Weierstrass pfunction via pari, the characteristic (%s) of the underlying field must be zero")
comment:6 Changed 9 years ago by
Commit:  1e692657b19d5ae80cd420e6a0c801a66cec6644 → 89726c59cdf83f3cee87e56806a3a86d8938df2c 

Branch pushed to git repo; I updated commit sha1. New commits:
89726c5  Fix exception formatting

comment:7 Changed 9 years ago by
Status:  needs_review → positive_review 

comment:8 Changed 9 years ago by
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
Description:  modified (diff) 

Status:  positive_review → needs_info 
I did some further cleaning up on a new branch u/pbruin/15855weierstrass_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 followup: 11 Changed 9 years ago by
Alternatively, repurpose this ticket as a cleanup ticket and leave the algorithm choice as it was.
comment:11 followup: 12 Changed 9 years ago by
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 Changed 9 years ago by
comment:13 Changed 9 years ago by
Description:  modified (diff) 

Summary:  Disable PARI algorithm for weierstrass_p in positive characteristic → Clean up weierstrass_p 
comment:14 Changed 9 years ago by
Commit:  89726c59cdf83f3cee87e56806a3a86d8938df2c → 0e4fff510181e974717899f802108be8877a9346 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0e4fff5  Clean up weierstrass_p function

comment:15 Changed 9 years ago by
Status:  needs_info → needs_review 

comment:16 followup: 18 Changed 9 years ago by
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 algorithmindependent ValueError
.)
comment:17 Changed 9 years ago by
Commit:  0e4fff510181e974717899f802108be8877a9346 → 666714eb35225661954ce037892f40d866cc04c3 

Branch pushed to git repo; I updated commit sha1. New commits:
666714e  Fix changelog entry for ell_wp

comment:18 Changed 9 years ago by
Status:  needs_review → positive_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 algorithmindependentValueError
.)
I have no idea, I don't know the mathematics well enough.
comment:19 Changed 9 years ago by
Branch:  u/jdemeyer/ticket/15855 → 666714eb35225661954ce037892f40d866cc04c3 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Clean up weierstrass_p function