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: sage-8.1
Component: number theory Keywords: Weil polynomial, sd91
Cc: Merged in:
Authors: David Zureick-Brown 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 kedlaya

  • Dependencies set to #23947

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).

comment:2 Changed 2 years ago by dzb

  • Branch set to u/dzb/add_test_for_weil_polynomials

comment:3 Changed 2 years ago by dzb

  • Authors set to David Zureick-Brown
  • Commit set to 4000398c7927e552e9d01e329d9218e230a89e83
  • Status changed from new to needs_review

Implemented is_weil_polynomial


New commits:

b59dffeImplement reciprocal transform, inverse reciprocal transform
314b45fRename inverse transform as trace polynomial
4000398Implemented is_weil_polynomial

comment:4 Changed 2 years ago by roed

  • 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 roed

  • Commit changed from 4000398c7927e552e9d01e329d9218e230a89e83 to ead122d8e4bc3b946af2aed1863ce2c3aec879d5

Made changes; if Kiran is happy then we can give this a positive review.


New commits:

33ea68bSmall fixes to docstring, add comment to code, use try/except on trace_polynomial rather than duplicating code
c5af2d4Fix docstring formatting, add doctests, fix error in cofactor of trace_polynomial
ead122dMerge branch 't/23947/reciprocal_transform' into t/23945/add_test_for_weil_polynomials

comment:6 Changed 2 years ago by kedlaya

  • Authors changed from David Zureick-Brown to Kiran Kedlaya, David Zureick-Brown

comment:7 Changed 2 years ago by kedlaya

  • Authors changed from Kiran Kedlaya, David Zureick-Brown to David Zureick-Brown
  • 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 kedlaya

  • 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 kedlaya

  • 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 kedlaya

  • 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:

f8b93d5Correct call syntax for trace_polynomial

comment:11 Changed 2 years ago by roed

Looks good to me. Positive review once tests pass.

comment:12 Changed 2 years ago by roed

  • Status changed from needs_review to positive_review

David Zureick-Brown 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 vbraun

  • 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 git

  • Commit changed from f8b93d5835e1fa0867ae31d972a1448ceb6db889 to 87721f94c74bf5a180d2c1fdf65671ebab6f8c97

Branch pushed to git repo; I updated commit sha1. New commits:

87721f9Correct docstring formatting

comment:15 Changed 2 years ago by kedlaya

  • 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 roed

  • 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 git

  • 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:

cdcc8cfAdd backticks to True in docstring

comment:18 Changed 2 years ago by kedlaya

I added the double backticks around True in the second line. In the one-sentence summary, I think the convention is not to use them.

comment:19 Changed 2 years ago by roed

  • 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.

Last edited 2 years ago by roed (previous) (diff)

comment:20 Changed 2 years ago by vbraun

  • Branch changed from u/kedlaya/add_test_for_weil_polynomials to cdcc8cfec1008982b0a91411fb70585062d8fbed
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.