Open d99bb657-85da-4e6a-9cf4-9041874f39c0 opened 6 years ago
Branch: u/rlmiller/bianca
New commits:
a7597f5 | 25745 periodic proportion hoomorphism |
Commit: a7597f5
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
Branch pushed to git repo; I updated commit sha1. New commits:
32ffa30 | 25745 updated tests and description |
Reviewer: Ben Hutz
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
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():
if n<=0 or not n in ZZ:
better as
n = ZZ(n)
if n <= 0:
sage: rows = [['a', 'b', 'c'], [100,2,3], [4,5,60]]
sage: table(rows)
NT. append([p, i+1, perRatio])
g=fp.cyclegraph()
PT. append([i, n, perRatio])
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.
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.
Given a homormorphism, a prime
p
, and a degreen
. Returns a table of the ratio of periodic points to the number of points in a field of sizep^n
, as it cycles through the range ofn
or through the primes up top
. One can organize table by ascending primes or by ascending degree. Skips the prime 2. Only works over finite fields.CC: @sagetrac-bthompson
Component: dynamics
Keywords: dynamical systems, sagedays90, days94, days90
Author: Rebecca Lauren Miller
Branch/Commit: u/rlmiller/bianca @
32ffa30
Reviewer: Ben Hutz
Issue created by migration from https://trac.sagemath.org/ticket/25745