Opened 4 years ago

Last modified 4 years ago

#25745 needs_work enhancement

Periodic Proportion Homomorphism over Finite Fields

Reported by: rlmiller Owned by:
Priority: minor Milestone: sage-8.3
Component: dynamics Keywords: dynamical systems, sagedays90, days94, days90
Cc: bthompson Merged in:
Authors: Rebecca Lauren Miller Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: u/rlmiller/bianca (Commits, GitHub, GitLab) Commit: 32ffa3092164fc097cd77beeac91eb93902fa926
Dependencies: Stopgaps:

Status badges

Description

Given a homormorphism, a prime p, and a degree n. Returns a table of the ratio of periodic

points to the number of points in a field of size p^n, as it cycles through the range of n or through the primes up to p. One can organize table by ascending primes or by ascending degree. Skips the prime 2. Only works over finite fields.

From paper by Zoë Bell, Karina Cho, Rebecca Lauren Miller, and Bianca Thompson. Author Rebecca Lauren Miller. Implimented at Sage Days 94, Algorithm developed as part of Sage Days 90.

Change History (5)

comment:1 Changed 4 years ago by rlmiller

  • Branch set to u/rlmiller/bianca

comment:2 Changed 4 years ago by rlmiller

  • Commit set to a7597f58e880ef76c1e3ce8f9e6fa95c0571a82d
  • Status changed from new to needs_review

New commits:

a7597f525745 periodic proportion hoomorphism

comment:3 Changed 4 years ago by bhutz

I can't look at this for a few days, so if someone else wants to review in the mean time go right ahead.

Just a couple quick comments

  • this seems to return a table not a homomorphism, so I would name the function differently.
  • It looks like you're expecting the field to be QQ (which is fine), but the docs should say where this should work and perhaps do a little input checking on the base field.
  • docstrings need to start with a single line description
  • paper should be added to reference file and referenced.
  • go ahead and put the input checking doctests into TESTS rather than EXAMPLES

comment:4 Changed 4 years ago by git

  • Commit changed from a7597f58e880ef76c1e3ce8f9e6fa95c0571a82d to 32ffa3092164fc097cd77beeac91eb93902fa926

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

32ffa3025745 updated tests and description

comment:5 Changed 4 years ago by bhutz

  • Reviewers set to Ben Hutz
  • Status changed from needs_review to needs_work

I haven't built and checked values yet, since there are a bunch of things from looking through the code first:

  • 'orderByPrime' -- camel case is reserved for classes, so don't use it in a parameter

or as a variable 'perRatio'

  • docs missing 's' -- Return a table of ratios of periodic point(s) to size of finite field.
  • seems like you might want to specify a list/range of primes/n as input.
f.periodic_proportion_table(p=[3,7,13],n=[1,3])

having the short cut

f.periodic_proportion_table(p=3, n=4)

mean all primes up to p and all exp up to n is fine

  • don't really understand the point of this test. The function isn't even in that class, so testing that the function isn't in the class is a little redundant since it will never fail.
sage: f.periodic_proportion_table(n=4)
Traceback (most recent call last):
...
AttributeError: 'DynamicalSystem_projective' object has no attribute 'periodic_proportion_table'
  • Inputs need a blank line between them.
  • I think the lines of the warning are not indented properly, but I didn't build the docs yet to check
  • for
    if is_prime(p) == False:
    

should be

if not is_prime():
  • for
    if n<=0 or not n in ZZ:
    

better as

n = ZZ(n)
if n <= 0:
  • there is a table constructor, so if you sy you return a table, you should be returning a table
sage: rows = [['a', 'b', 'c'], [100,2,3], [4,5,60]]
sage: table(rows)
  • space typos
    NT. append([p, i+1, perRatio])
    g=fp.cyclegraph()
    PT. append([i, n, perRatio])
    
  • the try/except seems to be just trying to catch indeterminacy locus issues and the error it raises seems to be specific to dimension 1.

cyclegraph can handle indeterminacy, so I don't really understand why you need this block. The warning you have seems to say that having an indeterminancy is ok, so I'm not sure what you really want here since the docs say you want a morphism? In my opinion, you are writing a function that is just returning the values to fill out the table, whether those values having any meaning is not really your business, so if it works for rational maps as well as morphisms, I see no reason to restrict to morphisms.

Also, I see no reason this can't be made to work in higher dimensions (other than it will be slow). You just need to adjust the cardinality calculation.

  • you're comments in the doctests are not formatted properly
comment::

test
  • the [4] mapping example needs the line wrapped it is much too long
  • TESTS:: -- needs the double colon
  • the big one that would help this function is to do these calculations in parallel. You are computing each p/n pair as completely independent, so this is best split into separate processes. The possible_periods() function in projective_ds gives a fairly simple example of this.
Note: See TracTickets for help on using tickets.