Opened 6 months ago

Closed 3 months ago

# Another error in height_difference_bound

Reported by: Owned by: Jing Guo minor sage-9.7 dynamics gsoc2022 Ben Hutz, Alexander Galarraga Jing Guo Ben Hutz N/A 97ae98d 97ae98d43829256ae7b7cfa509c2aa569bcd648d #34175

Similar to #33971, the height of a polynomial is not typically the max of the heights of the coefficients. In this application the height of the polynomial should be the height of the coefficients if they are thought of as a projective point (i.e., the height is independent of scaling). This is sometimes called the "projective" height of a polynomial. However, this function is computing `h(g_i)` as the max of the heights of the coefficients of `g_i`.

```for k in range(N + 1):
CoeffPolys = (CR.gen(k) ** D).lift(I)
h = max([c.global_height(prec) for g in CoeffPolys for c in (g).coefficients()])
maxh = max(maxh, h)
```

### comment:1 Changed 6 months ago by Ben Hutz

Description: modified (diff)

It may makes sense to first implement a height function for polynomials and then use that height function in height_difference_bound

### comment:2 Changed 5 months ago by Jing Guo

Branch: → u/gh-guojing0/33972_height_diff_bound → f8df80820dc7321dc9b18c9644c3b8315999670b

### comment:3 Changed 5 months ago by Jing Guo

Dependencies: → #34060

### comment:4 Changed 5 months ago by Jing Guo

Branch: u/gh-guojing0/33972_height_diff_bound → u/gh-guojing0/33972_height_bound f8df80820dc7321dc9b18c9644c3b8315999670b → ebaa377df21f419faa36f86c825ce8af8cb85aef

New commits:

 ​212ce1f `polynomial_element.pyx: First version of `global_height`` ​c642890 `multi_polynomial_libsingular.pyx: `global_height`` ​0c6a176 `More examples` ​c2242cb `Move `import` stmts to top and correct doc` ​4ac23b8 `remove `QQbar`` ​a94e199 `local_height and local_height_arch for polys` ​4f4fe17 `Correct doc` ​ebaa377 `Replace incorrect code with `global_height``

### comment:5 Changed 5 months ago by Jing Guo

Status: new → needs_review

### comment:6 Changed 5 months ago by Jing Guo

Branch: u/gh-guojing0/33972_height_bound → u/gh-guojing0/33972_hdb ebaa377df21f419faa36f86c825ce8af8cb85aef → a5e23b81d90ed9b9d893e6cf11c8b608ecaef356

Update with latest code from polynomial height.

New commits:

 ​c1b22a1 `34060: initial implementation of heights for polynomials` ​d5aad00 `34060: code cleanup and QQbar fixes` ​ad991a3 `34060: Number field and `prec` examples` ​a5e23b8 `33971: Commit with latest polynomial height code`

### comment:7 Changed 5 months ago by Jing Guo

Only failure (with actual return value `2.9957322...`):

```sage: P.<x,y> = ProjectiveSpace(QQ, 1)
sage: f = DynamicalSystem([5*x^2 + 3*x*y , y^2 + 3*x^2])
sage: f.height_difference_bound(prec=100)
5.3375380797013179737224159274
```

### comment:8 Changed 5 months ago by Ben Hutz

Reviewers: → Ben Hutz needs_review → needs_work

This change is wrong:

```-        CR = f.domain().coordinate_ring()
-        I = CR.ideal(f.defining_polynomials())
-        maxh = 0
-        for k in range(N + 1):
-            CoeffPolys = (CR.gen(k) ** D).lift(I)
-            h = max([c.global_height(prec) for g in CoeffPolys for c in (g).coefficients()])
-            maxh = max(maxh, h)
-        L = R((N + 1) * binomial(N + D - d, D - d)).log() + maxh
+        L = R((N + 1) * binomial(N + D - d, D - d)).log() + f.global_height(prec)
```

The error of the existing code is the line

```h = max([c.global_height(prec) for g in CoeffPolys for c in (g).coefficients()])
```

What is wrong is that it is taking the height of g to be the max of the height of its coefficients. You still need to take maxh to be the max of the heights of the CoeffPolys?, but the correction is to use the new height code (#34060) to compute the heights of each g.

### comment:9 Changed 5 months ago by git

Commit: a5e23b81d90ed9b9d893e6cf11c8b608ecaef356 → a3af7bab11fb37afa52b47e08cf67441623777d0

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

 ​31e2b34 `temp example` ​a3af7ba `33072: Works for most tests`

### comment:10 Changed 5 months ago by git

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​913fe15 `34175: global_height returns zero for zero poly` ​2aff55c `33972: Fix `height_difference_bound``

### comment:11 Changed 5 months ago by Jing Guo

Dependencies: #34060 → #34175 needs_work → needs_review

### comment:12 Changed 5 months ago by git

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

 ​b8a2b44 `34175: Return zero as real of specific precision` ​1ceb3d6 `Merge branch 'zero_height' into height_bound`

### comment:13 Changed 4 months ago by Ben Hutz

Status: needs_review → positive_review

This looks fine at this point. The difference in the examples is because of correcting the height of the g polynomials. They are monomials in these examples, so have (logarithmic) height 0 rather than the height of the coefficient.

So I'll mark this positive modulo any issues that show up when the dependency gets merged.

### comment:14 Changed 4 months ago by Volker Braun

Status: positive_review → needs_work
```**********************************************************************
File "src/sage/dynamics/arithmetic_dynamics/projective_ds.py", line 2116, in sage.dynamics.arithmetic_dynamics.projective_ds.?.height_difference_bound
Failed example:
f.height_difference_bound()
Expected:
11.0020998412042
Got:
10.3089526606443
**********************************************************************
File "src/sage/dynamics/arithmetic_dynamics/projective_ds.py", line 2128, in sage.dynamics.arithmetic_dynamics.projective_ds.?.height_difference_bound
Failed example:
f.height_difference_bound()
Expected:
11.0020998412042
Got:
11.3683039374269

q^CKilling test src/sage/dynamics/arithmetic_dynamics/projective_ds.py
----------------------------------------------------------------------
Doctests interrupted: 0/1 files tested
----------------------------------------------------------------------
```

### comment:15 Changed 4 months ago by Ben Hutz

I'm also getting these errors on a clean branch. The first one is in the example

```            sage: P.<x,y,z> = ProjectiveSpace(ZZ, 2)
sage: f = DynamicalSystem_projective([4*x^2 + 100*y^2, 210*x*y, 10000*z^2])
sage: f.height_difference_bound()
11.0020998412042
sage: f.normalize_coordinates()
sage: f.height_difference_bound()
10.3089526606443
```

It doesn't make sense to keep .normalize in here anymore as the height of the polynomials is independent of the normalization. The 10.308... is the correct value. So condense this example down to a single (correct) value.

The second failure is just the wrong value.

### comment:16 Changed 4 months ago by Jing Guo

Branch: u/gh-guojing0/33972_hdb → u/gh-guojing0/33972_stable 1ceb3d6da2e2ad209971436ab5bb0a059166a198 → 97ae98d43829256ae7b7cfa509c2aa569bcd648d needs_work → needs_review

Build and pass all tests on the new branch based on the latest `develop` branch.

New commits:

 ​97ae98d `33972 final version`

### comment:17 Changed 4 months ago by Ben Hutz

Status: needs_review → positive_review

This looks fine and the tests pass for me.

In the future it's better to edit the existing ticket to fix the issues rather than create a whole new branch that erases the history.

### comment:18 Changed 3 months ago by Volker Braun

Branch: u/gh-guojing0/33972_stable → 97ae98d43829256ae7b7cfa509c2aa569bcd648d → fixed positive_review → closed
Note: See TracTickets for help on using tickets.