Opened 4 years ago
Closed 4 years ago
#15855 closed enhancement (fixed)
Clean up weierstrass_p
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage6.2 
Component:  elliptic curves  Keywords:  
Cc:  cremona, pbruin  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Peter Bruin 
Report Upstream:  N/A  Work issues:  
Branch:  666714e (Commits)  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 4 years ago by
 Description modified (diff)
comment:2 Changed 4 years ago by
 Branch set to u/jdemeyer/ticket/15855
 Created changed from 02/24/14 12:39:24 to 02/24/14 12:39:24
 Modified changed from 02/24/14 13:29:43 to 02/24/14 13:29:43
comment:3 Changed 4 years ago by
 Commit set to 1e692657b19d5ae80cd420e6a0c801a66cec6644
comment:4 Changed 4 years ago by
 Status changed from new to needs_review
New commits:
1e69265  Clean up weierstrass_p function

comment:5 Changed 4 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 4 years ago by
 Commit changed from 1e692657b19d5ae80cd420e6a0c801a66cec6644 to 89726c59cdf83f3cee87e56806a3a86d8938df2c
Branch pushed to git repo; I updated commit sha1. New commits:
89726c5  Fix exception formatting

comment:7 Changed 4 years ago by
 Status changed from needs_review to positive_review
comment:8 Changed 4 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 4 years ago by
 Description modified (diff)
 Status changed from positive_review to 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 4 years ago by
Alternatively, repurpose this ticket as a cleanup ticket and leave the algorithm choice as it was.
comment:11 in reply to: ↑ 10 ; followup: ↓ 12 Changed 4 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 in reply to: ↑ 11 Changed 4 years ago by
comment:13 Changed 4 years ago by
 Description modified (diff)
 Summary changed from Disable PARI algorithm for weierstrass_p in positive characteristic to Clean up weierstrass_p
comment:14 Changed 4 years ago by
 Commit changed from 89726c59cdf83f3cee87e56806a3a86d8938df2c to 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 4 years ago by
 Status changed from needs_info to needs_review
comment:16 followup: ↓ 18 Changed 4 years ago by
 Reviewers set to 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 4 years ago by
 Commit changed from 0e4fff510181e974717899f802108be8877a9346 to 666714eb35225661954ce037892f40d866cc04c3
Branch pushed to git repo; I updated commit sha1. New commits:
666714e  Fix changelog entry for ell_wp

comment:18 in reply to: ↑ 16 Changed 4 years ago by
 Status changed from needs_review to 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 4 years ago by
 Branch changed from u/jdemeyer/ticket/15855 to 666714eb35225661954ce037892f40d866cc04c3
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Clean up weierstrass_p function