Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renaming rational_preperiodic_points() to all_preperiodic_points() #28213

Closed
EnderWannabe opened this issue Jul 18, 2019 · 31 comments
Closed

Renaming rational_preperiodic_points() to all_preperiodic_points() #28213

EnderWannabe opened this issue Jul 18, 2019 · 31 comments

Comments

@EnderWannabe
Copy link
Contributor

To be consistent with the new naming convention in ticket 28109, we rename rational_preperiodic_points() to all_preperiodic_points(). We also add a preperiod_points() method.

Component: dynamics

Keywords: SI2019, sd104

Author: Eric Zhu, Alex Galarraga, Bianca Thompson, Ben Hutz

Branch/Commit: b510b32

Reviewer: Jasmine Camero, Olivia Schwager, Anna Chlopecki, Simon Xu, Grayson Jorgenson, Juliano Levier-Gomes, Ben Hutz, Jamie Juul, Bella Tobin

Issue created by migration from https://trac.sagemath.org/ticket/28213

@EnderWannabe EnderWannabe added this to the sage-8.9 milestone Jul 18, 2019
@EnderWannabe
Copy link
Contributor Author

Branch: u/gh-EnderWannabe/28213

@EnderWannabe
Copy link
Contributor Author

New commits:

e221285started deprecation
0c5f507finished deprecation

@EnderWannabe
Copy link
Contributor Author

Commit: 0c5f507

@oliviaschwager
Copy link
Mannequin

oliviaschwager mannequin commented Jul 18, 2019

Reviewer: Jasmine Camero, Olivia Schwager

@bhutz
Copy link

bhutz commented Jul 18, 2019

comment:5

I don't think this is quite ready yet. Don't we want a 'preperiodic_points()' function as well as an option to specify the ring like you did in #28109?

@Zopherus

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2019

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

7a07a21added ring parameter to all_preperiodic_points
7c2f54cwrote perperiodic_points method
a54b88bbuilds, but does not run

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2019

Changed commit from 0c5f507 to a54b88b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2019

Changed commit from a54b88b to 2830a1d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 25, 2019

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

2830a1dworking preperiodic_points

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 30, 2019

Changed commit from 2830a1d to d451af4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 30, 2019

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

d451af4added tests and fixed documentation

@annanc2
Copy link
Mannequin

annanc2 mannequin commented Jul 30, 2019

comment:11
  • This line L = [t for t in L if t != 0] is not needed: it should not affect the subscheme.
  • X = PS.subscheme(L + list(dom.defining_polynomials())) - there are no examples where the domain is a proper subscheme of projective space. There should be an example illustrating when the domain is nontrivial.
  • Provide more explanation for what m and n are relative to each other.
  • The example below is pulled from the periodic_points function; it is a little slow as is. It would be nicer to find a faster version of this, but it does test the case where the domain is a nontrivial subscheme.
sage: R.<x> = QQ[]
sage: K.<u> = NumberField(x^2 - x + 3)
sage: P.<x,y,z> = ProjectiveSpace(K,2)
sage: X = P.subscheme(2*x-y)
sage: f = DynamicalSystem_projective([x^2-y^2, 2*(x^2-y^2), y^2-z^2], domain=X)
sage: f.preperiodic_points(2,2, R=QQbar)

@annanc2 annanc2 mannequin added s: needs work and removed s: needs review labels Jul 30, 2019
@annanc2
Copy link
Mannequin

annanc2 mannequin commented Jul 30, 2019

Changed reviewer from Jasmine Camero, Olivia Schwager to Jasmine Camero, Olivia Schwager, Anna Chlopecki, Simon Xu, Grayson Jorgenson, Juliano Levier-Gomes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 30, 2019

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

0d06e77added another example and cleaned up documentation

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 30, 2019

Changed commit from d451af4 to 0d06e77

@bhutz
Copy link

bhutz commented Jul 31, 2019

comment:15

Just one quick comment from a look through the branch.

In one function you use the keyword R, in the other the keyword ring. Those should be the same. If I recall correctly, you were using R for the similar periodic point functions, so I'd stick with that one.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 31, 2019

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

67ae944fixed keyword mismatch

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 31, 2019

Changed commit from 0d06e77 to 67ae944

@bhutz
Copy link

bhutz commented Nov 17, 2019

Changed branch from u/gh-EnderWannabe/28213 to u/bhutz/28213

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 17, 2019

Changed commit from 67ae944 to c427939

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 17, 2019

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

c42793928213: minor changes from review

@bhutz
Copy link

bhutz commented Nov 17, 2019

Changed reviewer from Jasmine Camero, Olivia Schwager, Anna Chlopecki, Simon Xu, Grayson Jorgenson, Juliano Levier-Gomes to Jasmine Camero, Olivia Schwager, Anna Chlopecki, Simon Xu, Grayson Jorgenson, Juliano Levier-Gomes, Ben Hutz, Jamie Juul, Bella Tobin

@bhutz
Copy link

bhutz commented Nov 17, 2019

Changed keywords from SI2019 to SI2019, sd104

@bhutz
Copy link

bhutz commented Nov 17, 2019

comment:19

made some minor updates from the review, mostly adding some additional doc tests.

@bhutz
Copy link

bhutz commented Nov 17, 2019

comment:20

I have an example that takes much longer than it should. Looks like it is because in all_preperiodic_points. there is no way to pass in the algorithm parameter

R.<t> = PolynomialRing(QQ)
K.<v> = NumberField(t^3 + 16*t^2 - 10496*t + 94208)
PS.<x,y> = ProjectiveSpace(K,1)
f = DynamicalSystem([x^2-29/16*y^2,y^2])

with the 'lifting' algorithm it is fast, with the default it is very slow.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 18, 2019

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

b510b3228213: add algorithm parameters to preperiodic

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 18, 2019

Changed commit from c427939 to b510b32

@bhutz
Copy link

bhutz commented Nov 18, 2019

comment:22

added parameters to all_preperiodic_points to make the given example possible in a reasonable amount of time. It is too long to add as an example. I've also updated the example that uses the bounds.

@bhutz
Copy link

bhutz commented Nov 18, 2019

Changed author from Eric Zhu, Alex Galarraga, Bianca Thompson to Eric Zhu, Alex Galarraga, Bianca Thompson, Ben Hutz

@vbraun
Copy link
Member

vbraun commented Nov 30, 2019

Changed branch from u/bhutz/28213 to b510b32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants