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

rational preperiodic points for projective morphisms #14219

Closed
bhutz opened this issue Mar 3, 2013 · 32 comments
Closed

rational preperiodic points for projective morphisms #14219

bhutz opened this issue Mar 3, 2013 · 32 comments

Comments

@bhutz
Copy link

bhutz commented Mar 3, 2013

Main algorithms from Hutz, "Determination of all rational preperiodic points for morphisms of PN", submitted

Also includes the last of the ICERM dynamics functionality. Building on #13130, #14217, #14218.

Implements all necessary functionality to determine QQ-rational periodic and preperiodic points for morphism of projective space in any dimension. This includes multipliers, hensel lifting, height difference bound, etc.

Apply:

Depends on #14218

Component: algebraic geometry

Keywords: dynamics, sage-days55

Author: Ben Hutz

Reviewer: Vincent Delecroix, Adam Towsley

Merged: sage-5.13.beta4

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

@bhutz bhutz added this to the sage-5.9 milestone Mar 3, 2013
@bhutz bhutz self-assigned this Mar 3, 2013
@fchapoton

This comment has been minimized.

@fchapoton
Copy link
Contributor

comment:1

Attachment: rational_preperiodic.patch.gz

@fchapoton
Copy link
Contributor

Changed dependencies from 14128 to #14128

@fchapoton
Copy link
Contributor

Changed dependencies from #14128 to #14218

@bhutz

This comment has been minimized.

@bhutz
Copy link
Author

bhutz commented Apr 15, 2013

comment:5

Finished cleaning it up. Ready for review.

Changed the milestone to something more realistic.

@bhutz bhutz modified the milestones: sage-5.9, sage-5.10 Apr 15, 2013
@fchapoton
Copy link
Contributor

comment:6

apply trac_14219_rational_preperiodic.patch

@fchapoton
Copy link
Contributor

comment:7

There is one doctest missing.

@fchapoton
Copy link
Contributor

Work Issues: doctest missing

@fchapoton
Copy link
Contributor

Changed keywords from none to dynamics

@bhutz
Copy link
Author

bhutz commented Aug 23, 2013

comment:9

Fixed the missing doctest.

This passes all tests on my machine for sage 5.11.

@bhutz
Copy link
Author

bhutz commented Aug 23, 2013

Changed work issues from doctest missing to none

@bhutz
Copy link
Author

bhutz commented Aug 23, 2013

comment:10

apply trac_14219_rational_preperiodic.patch

@bhutz
Copy link
Author

bhutz commented Sep 4, 2013

comment:11

apply trac_14219_rational_preperiodic.patch

@bhutz
Copy link
Author

bhutz commented Oct 20, 2013

comment:12

apply trac_14219_rational_preperiodic.patch

@bhutz
Copy link
Author

bhutz commented Oct 21, 2013

comment:13

apply trac_14219_rational_preperiodic.patch

@videlec
Copy link
Contributor

videlec commented Nov 7, 2013

optimize the root order computation over finite fields

@videlec
Copy link
Contributor

videlec commented Nov 7, 2013

comment:14

Attachment: trac_14219-optimize_root_order_computation.patch.gz

Hi there,

Some quick comments (I am not the person designed to review the mathematics in there):

  • the patch does not apply on sage-5.13.beta2
  • there are many trailing whitescaces that need to be removed
  • the indentation must be 4 spaces wide (it is 3 in many places)
  • sage convention is that CamelCaseConvention is somewhat reserved to classes and it would be better to not use it as variables names (I think this is too much pain to change it right now)
  • in the examples one should use "a = bla()" instead of "a=bla()"

We also discussed a very specific issue in the code (line 2588 to 2601). The attachment: trac_14219-optimize_root_order_computation.patch proposes a much direct version with, as a consequence, a little bit of speedup.

@bhutz
Copy link
Author

bhutz commented Nov 8, 2013

Changed keywords from dynamics to dynamics, sage-days55

@bhutz
Copy link
Author

bhutz commented Nov 8, 2013

Reviewer: Vincent Delecroix

@bhutz
Copy link
Author

bhutz commented Nov 8, 2013

comment:16

The change for the eigenvalue computation looks fine, I've folded that into the main patch.

I removed the whitespace and added the spaces around the = signs.

I also adjusted the variable naming as that wasn't too tedious.

I couldn't find any 3 space indents, could you point out where those are.

@videlec
Copy link
Contributor

videlec commented Nov 8, 2013

comment:17

To be modified in projective_morphism.py

  1. trailing whitespaces in at lines 1663, 1754, 1954, 1971, 1982, 1985, 1992, 1998, 2085, 2106, 2158, 2169, 2221, 2257, 2273, 2317, 2352,

  2. why do you use Hom(P,P) instead of End(P) ?

  3. problem of upper case/lower case at line 1746 (Periods instead of periods)

  4. prefer (line 1743-1746)

return sorted(periods)

instead of

periods=list(periods)
periods.sort()
return(periods)
  1. in method dynatomic_polynomial the argument is called period but is refered in the documentation as n. Moreover the specification of arguments are given too late. In the examples it might be good to check that the dynatomic polynomial actually gives you solution!
sage: P.<x,y> = ProjectiveSpace(QQ,1)
sage: H = Hom(P,P)
sage: f = H([x^2+y^2,y^2])
sage: f.dynatomic_polynomial(2)
x^2 + x*y + 2*y^2
sage: K.<c> = NumberField(X^2 + X + 2, 'c')
sage: PP = P.change_ring(K)
sage: ff = f.change_ring(K)
sage: p = PP((c,1))
sage: ff(ff(p)) == p

Here I get troubles with coercions (this is why I defined PP and ff): there should be some canonical coercions as there is for polynomials (this will go in another ticket)

sage: R.<x> = PolynomialRing(QQ,'x')
sage: K.<c> = NumberField(x^2 + 3*x + 1, 'c')
sage: P = x^6 + 3*x + 19
sage: P(c)
-141*c - 36
  1. The rational for the choice of putting methods in projective_point.py instead in projective_morphism.py is not at all clear to me. Why orbit, canonical_heights, etc methods of points and not of morphisms ?

  2. docbuild troubles
    [schemes ] /opt/sage/local/lib/python2.7/site-packages/sage/schemes/projective/projective_morphism.py:docstring of sage.schemes.projective.projective_morphism.SchemeMorphism_polynomial_projective_space_field.rational_periodic_points:4: WARNING: Inline literal start-string without end-string.
    [schemes ] /opt/sage/local/lib/python2.7/site-packages/sage/schemes/projective/projective_morphism.py:docstring of sage.schemes.projective.projective_morphism.SchemeMorphism_polynomial_projective_space_field.rational_periodic_points:72: WARNING: Inline strong start-string without end-string.
    [schemes ] /opt/sage/local/lib/python2.7/site-packages/sage/schemes/projective/projective_morphism.py:docstring of sage.schemes.projective.projective_morphism:7: WARNING: Bullet list ends without a blank line; unexpected unindent.
    [schemes ] /opt/sage/local/lib/python2.7/site-packages/sage/schemes/projective/projective_morphism.py:docstring of sage.schemes.projective.projective_morphism.SchemeMorphism_polynomial_projective_space_field.rational_preperiodic_points:4: WARNING: Inline literal start-string without end-string.
    [schemes ] /opt/sage/local/lib/python2.7/site-packages/sage/schemes/projective/projective_morphism.py:docstring of sage.schemes.projective.projective_morphism:12: WARNING: Bullet list ends without a blank line; unexpected unindent.

  3. I have serious trouble concerning the green function and canonical heights of points (I know that this was implemented in another ticket). In those two methods, there is no care about the fact that there are errors introduced by the fact that the floating points you are working with are with fixed precision. In particular, with floating points, the result of x+y is not the sum x + y but only an approximation of it. I really think that it should be reimplemented either using real interval fields (it will make it slower but at least you get a proven answer).

@bhutz
Copy link
Author

bhutz commented Nov 8, 2013

comment:18

fixed: 1,2,3,4,7

5 - is two separate issues for other tickets

6 - since the methods depends on both points I didn't think it mattered. I can see a preference for the code in morphism. If this is a strong preference for Sage users, then open a separate ticket to move this.

8 - also an issue for a separate ticket

@sagetrac-atowsley
Copy link
Mannequin

sagetrac-atowsley mannequin commented Nov 10, 2013

comment:20

The multiplier (and multipliermod) functions should have a check that the point P does have period n.

@sagetrac-atowsley
Copy link
Mannequin

sagetrac-atowsley mannequin commented Nov 10, 2013

comment:21

Here are a few more comments:

  1. Please remove debugging code from lines 1865-1968.

  2. It would be helpful to have some more comments to the code for the Henzel lifting to make it easier to read.

  3. Use is_endomorphism() where possible. Lines 2044, 2138, and 2199.

  4. I think it would be clearer if the rational_preimages and all_rational_preimages were written in that order. That is the reverse of how they appear now.

  5. Line 2145 the set 'preperiodic' as the name is misleading. Maybe 'all_preimages' would be better?

  6. More comments on the Groebner basis code would be nice.

  7. You should consider caching the functions rational_preperiodic_points and rational_periodic_points.

@bhutz
Copy link
Author

bhutz commented Nov 10, 2013

comment:22

Multiplier check added for the (2)-previous comment. I added a parameter to specify whether to do this or not as this function is called many times for rational_periodic and it would be better not to check if you known the input is good.

1,2,3,4,5,6 all done

  1. Not possible since the points are rational projective points and they are not hashable (only hashable over finite fields)

I also changed the mrange to xmrange in lift_to_rational to help improve the higher dimension performance.

@bhutz
Copy link
Author

bhutz commented Nov 10, 2013

changes made

@sagetrac-atowsley
Copy link
Mannequin

sagetrac-atowsley mannequin commented Nov 12, 2013

Changed reviewer from Vincent Delecroix to Vincent Delecroix, Adam Towsley

@sagetrac-atowsley
Copy link
Mannequin

sagetrac-atowsley mannequin commented Nov 12, 2013

comment:23

Attachment: trac_14219_rational_preperiodic.patch.gz

All tests passed. Everything looks good.

@jdemeyer
Copy link

Merged: sage-5.13.beta4

@jdemeyer
Copy link

Changed author from Ben Hutz to Benjamin Hutz

@fchapoton
Copy link
Contributor

Changed author from Benjamin Hutz to Ben Hutz

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