Opened 8 months ago
Closed 8 months ago
#15855 closed enhancement (fixed)
Clean up weierstrass_p
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.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 jdemeyer)
Fix the precision bound for the quadratic algorithm and clean up the code.
Change History (19)
comment:1 Changed 8 months ago by jdemeyer
- Description modified (diff)
comment:2 Changed 8 months ago by jdemeyer
- Branch set to u/jdemeyer/ticket/15855
- Created changed from 02/24/14 04:39:24 to 02/24/14 04:39:24
- Modified changed from 02/24/14 05:29:43 to 02/24/14 05:29:43
comment:3 Changed 8 months ago by git
- Commit set to 1e692657b19d5ae80cd420e6a0c801a66cec6644
comment:4 Changed 8 months ago by jdemeyer
- Status changed from new to needs_review
New commits:
1e69265 | Clean up weierstrass_p function |
comment:5 Changed 8 months ago by pbruin
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 8 months ago by git
- Commit changed from 1e692657b19d5ae80cd420e6a0c801a66cec6644 to 89726c59cdf83f3cee87e56806a3a86d8938df2c
Branch pushed to git repo; I updated commit sha1. New commits:
89726c5 | Fix exception formatting |
comment:7 Changed 8 months ago by pbruin
- Status changed from needs_review to positive_review
comment:8 Changed 8 months ago by pbruin
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 8 months ago by pbruin
- Description modified (diff)
- Status changed from positive_review to needs_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 follow-up: ↓ 11 Changed 8 months ago by jdemeyer
Alternatively, repurpose this ticket as a cleanup ticket and leave the algorithm choice as it was.
comment:11 in reply to: ↑ 10 ; follow-up: ↓ 12 Changed 8 months ago by 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.
comment:12 in reply to: ↑ 11 Changed 8 months ago by pbruin
comment:13 Changed 8 months ago by jdemeyer
- Description modified (diff)
- Summary changed from Disable PARI algorithm for weierstrass_p in positive characteristic to Clean up weierstrass_p
comment:14 Changed 8 months ago by git
- 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 8 months ago by jdemeyer
- Status changed from needs_info to needs_review
comment:16 follow-up: ↓ 18 Changed 8 months ago by pbruin
- 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 algorithm-independent ValueError.)
comment:17 Changed 8 months ago by git
- 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 8 months ago by jdemeyer
- 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 algorithm-independent ValueError.)
I have no idea, I don't know the mathematics well enough.
comment:19 Changed 8 months ago by vbraun
- 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: