Opened 5 years ago
Closed 5 years ago
#23945 closed enhancement (fixed)
Add test for Weil polynomials
Reported by:  Kiran 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, GitHub, GitLab)  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 5 years ago by
Dependencies:  → #23947 

comment:2 Changed 5 years ago by
Branch:  → u/dzb/add_test_for_weil_polynomials 

comment:3 Changed 5 years ago by
Authors:  → David ZureickBrown 

Commit:  → 4000398c7927e552e9d01e329d9218e230a89e83 
Status:  new → needs_review 
comment:4 Changed 5 years ago by
Branch:  u/dzb/add_test_for_weil_polynomials → u/roed/add_test_for_weil_polynomials 

comment:5 Changed 5 years ago by
Commit:  4000398c7927e552e9d01e329d9218e230a89e83 → 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 5 years ago by
Authors:  David ZureickBrown → Kiran Kedlaya, David ZureickBrown 

comment:7 Changed 5 years ago by
Authors:  Kiran Kedlaya, David ZureickBrown → David ZureickBrown 

Reviewers:  → Kiran Kedlaya, David Roe 
Status:  needs_review → positive_review 
Looks good to me.
comment:8 Changed 5 years ago by
Status:  positive_review → needs_work 

Sorry, backing out the positive review due to some failing doctests. More soon.
comment:9 Changed 5 years ago by
Branch:  u/roed/add_test_for_weil_polynomials → u/kedlaya/add_test_for_weil_polynomials 

comment:10 Changed 5 years ago by
Commit:  ead122d8e4bc3b946af2aed1863ce2c3aec879d5 → f8b93d5835e1fa0867ae31d972a1448ceb6db889 

Status:  needs_work → 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:12 Changed 5 years ago by
Status:  needs_review → 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 5 years ago by
Status:  positive_review → 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 5 years ago by
Commit:  f8b93d5835e1fa0867ae31d972a1448ceb6db889 → 87721f94c74bf5a180d2c1fdf65671ebab6f8c97 

Branch pushed to git repo; I updated commit sha1. New commits:
87721f9  Correct docstring formatting

comment:15 Changed 5 years ago by
Status:  needs_work → needs_review 

Looks like there was some dodgy formatting in the docstring. Let's see if I managed to fix it.
comment:16 Changed 5 years ago by
Status:  needs_review → positive_review 

Seems fine, though I think it's better if True has double backticks around it.
comment:17 Changed 5 years ago by
Commit:  87721f94c74bf5a180d2c1fdf65671ebab6f8c97 → cdcc8cfec1008982b0a91411fb70585062d8fbed 

Status:  positive_review → 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 5 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 5 years ago by
Status:  needs_review → 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 5 years ago by
Branch:  u/kedlaya/add_test_for_weil_polynomials → cdcc8cfec1008982b0a91411fb70585062d8fbed 

Resolution:  → fixed 
Status:  positive_review → 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).