Opened 2 years ago
Closed 2 years ago
#23945 closed enhancement (fixed)
Add test for Weil polynomials
Reported by:  kedlaya  Owned by:  

Priority:  minor  Milestone:  sage8.1 
Component:  number theory  Keywords:  Weil polynomial, sd91 
Cc:  Merged in:  
Authors:  David ZureickBrown  Reviewers:  Kiran Kedlaya, David Roe 
Report Upstream:  N/A  Work issues:  
Branch:  cdcc8cf (Commits)  Commit:  cdcc8cfec1008982b0a91411fb70585062d8fbed 
Dependencies:  #23947  Stopgaps: 
Description
We are adding a function that tests whether a given polynomial is a Weil polynomial for a given prime power q, i.e., whether its complex roots all have absolute value sqrt(q)
.
Change History (20)
comment:1 Changed 2 years ago by
 Dependencies set to #23947
comment:2 Changed 2 years ago by
 Branch set to u/dzb/add_test_for_weil_polynomials
comment:3 Changed 2 years ago by
 Commit set to 4000398c7927e552e9d01e329d9218e230a89e83
 Status changed from new to needs_review
comment:4 Changed 2 years ago by
 Branch changed from u/dzb/add_test_for_weil_polynomials to u/roed/add_test_for_weil_polynomials
comment:5 Changed 2 years ago by
 Commit changed from 4000398c7927e552e9d01e329d9218e230a89e83 to ead122d8e4bc3b946af2aed1863ce2c3aec879d5
Made changes; if Kiran is happy then we can give this a positive review.
New commits:
33ea68b  Small fixes to docstring, add comment to code, use try/except on trace_polynomial rather than duplicating code

c5af2d4  Fix docstring formatting, add doctests, fix error in cofactor of trace_polynomial

ead122d  Merge branch 't/23947/reciprocal_transform' into t/23945/add_test_for_weil_polynomials

comment:6 Changed 2 years ago by
comment:7 Changed 2 years ago by
 Reviewers set to Kiran Kedlaya, David Roe
 Status changed from needs_review to positive_review
Looks good to me.
comment:8 Changed 2 years ago by
 Status changed from positive_review to needs_work
Sorry, backing out the positive review due to some failing doctests. More soon.
comment:9 Changed 2 years ago by
 Branch changed from u/roed/add_test_for_weil_polynomials to u/kedlaya/add_test_for_weil_polynomials
comment:10 Changed 2 years ago by
 Commit changed from ead122d8e4bc3b946af2aed1863ce2c3aec879d5 to f8b93d5835e1fa0867ae31d972a1448ceb6db889
 Status changed from needs_work to needs_review
I fixed the issue with the doctests. Also, I don't like redefining self
within a method definition.
All tests now pass on k8s. Since I touched the code last, someone other than me should check my work and then set a positive review.
New commits:
f8b93d5  Correct call syntax for trace_polynomial

comment:11 Changed 2 years ago by
Looks good to me. Positive review once tests pass.
comment:12 Changed 2 years ago by
 Status changed from needs_review to positive_review
David ZureickBrown isn't a trusted author, so patchbot won't run. I ran tests on k8s with no failures. Positive review.
comment:13 Changed 2 years ago by
 Status changed from positive_review to needs_work
[dochtml] OSError: [polynomia] docstring of sage.rings.polynomial.polynomial_element.Polynomial.is_weil_polynomial:10: WARNING: Bullet list ends without a blank line; unexpected unindent.
comment:14 Changed 2 years ago by
 Commit changed from f8b93d5835e1fa0867ae31d972a1448ceb6db889 to 87721f94c74bf5a180d2c1fdf65671ebab6f8c97
Branch pushed to git repo; I updated commit sha1. New commits:
87721f9  Correct docstring formatting

comment:15 Changed 2 years ago by
 Status changed from needs_work to needs_review
Looks like there was some dodgy formatting in the docstring. Let's see if I managed to fix it.
comment:16 Changed 2 years ago by
 Status changed from needs_review to positive_review
Seems fine, though I think it's better if True has double backticks around it.
comment:17 Changed 2 years ago by
 Commit changed from 87721f94c74bf5a180d2c1fdf65671ebab6f8c97 to cdcc8cfec1008982b0a91411fb70585062d8fbed
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
cdcc8cf  Add backticks to True in docstring

comment:18 Changed 2 years ago by
I added the double backticks around True in the second line. In the onesentence summary, I think the convention is not to use them.
comment:19 Changed 2 years ago by
 Status changed from needs_review to positive_review
Good enough, though I think the convention would be Return whether this is a Weil polynomial
rather than Return True if this is a Weil polynomial
.
comment:20 Changed 2 years ago by
 Branch changed from u/kedlaya/add_test_for_weil_polynomials to cdcc8cfec1008982b0a91411fb70585062d8fbed
 Resolution set to fixed
 Status changed from positive_review to closed
One easy way to do this would be to combine the "inverse reciprocal transform" from #23947 with the existing
all_roots_in_interval
method of polynomials (which wraps Pari's implementation of Sturm sequences).