Opened 6 months ago

Closed 4 months ago

#33971 closed defect (fixed)

Height of a dynamical system is wrong

Reported by: Jing Guo Owned by: Jing Guo
Priority: minor Milestone: sage-9.7
Component: dynamics Keywords: gsoc2022
Cc: Ben Hutz, Alexander Galarraga Merged in:
Authors: Jing Guo Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: d24eea8 (Commits, GitHub, GitLab) Commit: d24eea822da6c3676b3194898da152828f3ee46b
Dependencies: Stopgaps:

Status badges

Description (last modified by Ben Hutz)

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.

Change History (30)

comment:1 Changed 6 months ago by Jing Guo

Description: modified (diff)
Type: enhancementtask

comment:2 Changed 6 months ago by Ben Hutz

Description: modified (diff)

comment:3 Changed 6 months ago by Ben Hutz

Type: taskdefect

comment:4 Changed 6 months ago by Jing Guo

Branch: u/gh-guojing0/33971_dyn_sys_height
Commit: 5fb2a6ea44400e469caee82748cf863ca0c5f724
Owner: set to Jing Guo

comment:5 Changed 6 months ago by git

Commit: 5fb2a6ea44400e469caee82748cf863ca0c5f724a2892e00569fe3b7731d20b04511f94b3ec1cbf3

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

a2892e0polynomial_element_generic.py: Add `height` fn

comment:6 Changed 6 months ago by git

Commit: a2892e00569fe3b7731d20b04511f94b3ec1cbf30d582710e18af55d4c7b3626a12237e322a80647

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

0d58271multi_polynomial_element.py and polynomial_element_generic.py: Implement

comment:7 Changed 6 months ago by Alexander Galarraga

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.

comment:8 Changed 6 months ago by Jing Guo

Branch: u/gh-guojing0/33971_dyn_sys_height
Commit: 0d582710e18af55d4c7b3626a12237e322a80647

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

comment:9 Changed 5 months ago by Jing Guo

Branch: u/gh-guojing0/33971_ds_height
Commit: 38425b51bcdc7fd556d8bd54f488d4e0a4096b14

New commits:

38425b5projective_morphism.py: Debug `global_height` fn

comment:10 Changed 5 months ago by git

Commit: 38425b51bcdc7fd556d8bd54f488d4e0a4096b14c7187f42ed44c13ced8458c9bdf8a038f3701f76

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

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

comment:11 Changed 5 months ago by Jing Guo

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.

Last edited 5 months ago by Jing Guo (previous) (diff)

comment:12 Changed 5 months ago by Jing Guo

Status: newneeds_review

comment:13 Changed 5 months ago by Ben Hutz

Reviewers: Ben Hutz
Status: needs_reviewneeds_work

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.

comment:14 Changed 5 months ago by git

Commit: c7187f42ed44c13ced8458c9bdf8a038f3701f76a182698d195cfb07cb24d7d1f48da93fd36aa0ff

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

a182698Debug `global_height` for proj morphism

comment:15 Changed 5 months ago by git

Commit: a182698d195cfb07cb24d7d1f48da93fd36aa0ffeb1d7918cc48c5c111f3031b1460ff09be272de0

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

eb1d791More doc

comment:16 Changed 5 months ago by git

Commit: eb1d7918cc48c5c111f3031b1460ff09be272de071c8807bf1210d8f3c51c855b87306a8c6ef97e7

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

71c8807Correct impl of `global_height` for affine morphism

comment:17 Changed 5 months ago by git

Commit: 71c8807bf1210d8f3c51c855b87306a8c6ef97e730dd5f26d32036b41f7fbbb35e2e6687a9186cdf

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

30dd5f2local height for affine morphism

comment:18 Changed 5 months ago by git

Commit: 30dd5f26d32036b41f7fbbb35e2e6687a9186cdf28369c6885e4cadad8bad7055aaac21bbdb600ec

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

28369c6local_height_arch for affine morphism

comment:19 Changed 5 months ago by Jing Guo

Status: needs_workneeds_review

The things in bhutz's reviews are fixed. The three height functions (global_height, local_height, and local_height_arch) for affine morphisms are implemented as well.

comment:20 Changed 5 months ago by Ben Hutz

Status: needs_reviewneeds_work
  • 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.
    

comment:21 Changed 5 months ago by git

Commit: 28369c6885e4cadad8bad7055aaac21bbdb600ec0a617700847eff2c4f3afb99adf80541bc4cb100

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

0a61770Correct doc

comment:22 Changed 5 months ago by git

Commit: 0a617700847eff2c4f3afb99adf80541bc4cb10094a28058512d44978f57d228b7d1c56f1563e493

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

94a280533971: Correct doc

comment:23 Changed 5 months ago by Jing Guo

Status: needs_workneeds_review

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.

comment:24 Changed 5 months ago by git

Commit: 94a28058512d44978f57d228b7d1c56f1563e493ab35cbc1fbda7fa4c05c67d63c949440bf0c1aef

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

ab35cbc33971: Correct `prec` and relevent examples

comment:25 Changed 5 months ago by Ben Hutz

Status: needs_reviewneeds_work

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

comment:26 Changed 5 months ago by git

Commit: ab35cbc1fbda7fa4c05c67d63c949440bf0c1aefd24eea822da6c3676b3194898da152828f3ee46b

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

d24eea833971: Allow domain and codomain have diff dim's

comment:27 Changed 5 months ago by Jing Guo

Status: needs_workneeds_review

The two examples are added as parts of the EXAMPLES.

comment:28 Changed 5 months ago by Ben Hutz

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.

comment:29 Changed 5 months ago by Ben Hutz

Status: needs_reviewpositive_review

comment:30 Changed 4 months ago by Volker Braun

Branch: u/gh-guojing0/33971_ds_heightd24eea822da6c3676b3194898da152828f3ee46b
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.