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

Height of a dynamical system is wrong #33971

Closed
guojing0 opened this issue Jun 10, 2022 · 45 comments
Closed

Height of a dynamical system is wrong #33971

guojing0 opened this issue Jun 10, 2022 · 45 comments

Comments

@guojing0
Copy link
Contributor

The height of a dynamical system is currently calculated as the max of the heights of the coefficients, which is incorrect. To avoid writing a complicated formula, the correct height can be thought of as treating the coefficients of all coordinate functions as one big projective point and taking the height of that. For example

P.<x,y>=ProjectiveSpace(QQ,1)
f=DynamicalSystem([1/25*x^2 + 25/3*x*y+y^2,1*y^2])
exp(f.global_height())

gives 25, but should be 625.

CC: @bhutz @EnderWannabe

Component: dynamics

Keywords: gsoc2022

Author: Jing Guo

Branch/Commit: d24eea8

Reviewer: Ben Hutz

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

@guojing0 guojing0 added this to the sage-9.7 milestone Jun 10, 2022
@guojing0

This comment has been minimized.

@bhutz

This comment has been minimized.

@bhutz bhutz added the t: bug label Jun 10, 2022
@guojing0
Copy link
Contributor Author

Branch: u/gh-guojing0/33971_dyn_sys_height

@guojing0
Copy link
Contributor Author

Commit: 5fb2a6e

@guojing0 guojing0 self-assigned this Jun 16, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2022

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

a2892e0polynomial_element_generic.py: Add `height` fn

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2022

Changed commit from 5fb2a6e to a2892e0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 22, 2022

Changed commit from a2892e0 to 0d58271

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 22, 2022

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

0d58271multi_polynomial_element.py and polynomial_element_generic.py: Implement

@EnderWannabe
Copy link
Contributor

comment:7

These changes are meant to add a height function for polynomials. As such, they should have their own ticket, as this ticket is meant to address dynamical systems specifically.

@guojing0
Copy link
Contributor Author

comment:8

As suggested by Alex, a new ticket #34060 is created.

@guojing0
Copy link
Contributor Author

Changed commit from 0d58271 to none

@guojing0
Copy link
Contributor Author

Changed branch from u/gh-guojing0/33971_dyn_sys_height to none

@guojing0
Copy link
Contributor Author

Branch: u/gh-guojing0/33971_ds_height

@guojing0
Copy link
Contributor Author

Commit: 38425b5

@guojing0
Copy link
Contributor Author

New commits:

38425b5projective_morphism.py: Debug `global_height` fn

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 29, 2022

Changed commit from 38425b5 to c7187f4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 29, 2022

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

c7187f4projective_morphism.py: New example and move `import` inside

@guojing0
Copy link
Contributor Author

comment:11

The return values of the examples in the docstring of global_height in projective_morphism.py are same as before, except for the first one (current result is something like 20.83484...):

sage: P.<x,y> = ProjectiveSpace(QQ,1)
sage: H = Hom(P,P)
sage: f = H([1/1331*x^2+1/4000*y^2, 210*x*y]);
sage: f.global_height()
8.29404964010203

I also added the example from description to the documentation.

@bhutz
Copy link

bhutz commented Jun 30, 2022

comment:13

A couple issues here

  • The dimension of the projective space is off by 1. It should be
P = ProjectiveSpace(K, len(coeffs)-1)
  • I'd suggest having one less variable defined
proj_point = P.point(coeffs)
return proj_point.global_height()

to

return P.point(coeffs).global_height()
  • correct the doc string to describe what this height is actually doing. (i.e., this is the global height of the coefficents as a projective point)

  • There seems to be an issue with scaling. The definition of height should be independent of the representative, so these should return the same height. I suggest adding an example for this too. There is an issue somewhere as these are giving different values.

P.<x,y>=ProjectiveSpace(QQ,1)
f=DynamicalSystem([1/25*x^2 + 25/3*x*y+y^2,1*y^2])
print(f.global_height())
c=10000
f.scale_by(c)
f.global_height()
  • These should be implemented for Affine Dynamical systems too. Careful here that you construct the correct projective point; it may be simplest to homogenize the dynamical system first and then call the projective version (don't forget the case of morphisms between spaces of different dimensions).

  • The local_height and local_height_arch seem not to be implemented for affine morphisms at all yet.

@bhutz
Copy link

bhutz commented Jun 30, 2022

Reviewer: Ben Hutz

@bhutz
Copy link

bhutz commented Jul 5, 2022

comment:20
  • add description to the affine one that you are taking the height of the homogenization.

-doc test failures:

1 item had failures:
   3 of  32 in sage.schemes.projective.projective_morphism.SchemeMorphism_polynomial_projective_space.global_height
    [612 tests, 3 failures, 4.79 s]

I did not run the full set of doc tests, so be sure to do so.

  • docs fail to build
OSError: /home/ben/sage-dev/local/var/lib/sage/venv-python3.10.3/lib/python3.10/site-packages/sage/schemes/projective/projective_morphism.py:docstring of sage.schemes.projective.projective_morphism.SchemeMorphism_polynomial_projective_space.global_height:60: WARNING: Literal block expected; none found.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2022

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

0a61770Correct doc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2022

Changed commit from 28369c6 to 0a61770

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2022

Changed commit from 0a61770 to 94a2805

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2022

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

94a280533971: Correct doc

@guojing0
Copy link
Contributor Author

guojing0 commented Jul 9, 2022

comment:23

The status badge says that building documentation is successful. It passed all the tests after I ran ./sage -t affine_morphism.py and ./sage -t projective_morphism.py.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2022

Changed commit from 94a2805 to ab35cbc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2022

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

ab35cbc33971: Correct `prec` and relevent examples

@bhutz
Copy link

bhutz commented Jul 11, 2022

comment:25

The functionality works when the domain and codomain have the same dimension, but not when they do not.

P.<x,y,z> = ProjectiveSpace(QQ,2)
A.<z,w>=ProjectiveSpace(QQ,1)
H = Hom(P,A)
f = H([1/1331*x^2 + 4000*y*z,y^2])
f.global_height()
P.<x,y> = AffineSpace(QQ,2)
A.<z>=AffineSpace(QQ,1)
H = Hom(P,A)
f = H([1/1331*x^2 + 4000*y])
f.global_height()

This example no longer makes sense since normalization does not have an effect on the height. Delete the text and the second half of the example.

        This function does not automatically normalize::

            sage: P.<x,y,z> = ProjectiveSpace(ZZ,2)
            sage: H = Hom(P,P)
            sage: f = H([4*x^2 + 100*y^2, 210*x*y, 10000*z^2]);
            sage: f.global_height()
            8.51719319141624
            sage: f.normalize_coordinates()
            sage: f.global_height()
            8.51719319141624

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 12, 2022

Changed commit from ab35cbc to d24eea8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 12, 2022

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

d24eea833971: Allow domain and codomain have diff dim's

@guojing0
Copy link
Contributor Author

comment:27

The two examples are added as parts of the EXAMPLES.

@bhutz
Copy link

bhutz commented Jul 12, 2022

comment:28

This is passing my tests now. I'd like to have the patchbot pick it up before I marked it positive. Let's give it a couple days.

@vbraun
Copy link
Member

vbraun commented Aug 1, 2022

Changed branch from u/gh-guojing0/33971_ds_height to d24eea8

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