Serious errors in heights of projective points over number fields

[JohnCremona]

This was raised at Ask Sage question 55698.

Number fields have an iterator to yield all elements of bounded global height. This is used in an incorrect way to enumerate points of bounded height on projective space: in schemes.projective.projective_space the iterator points_of_bounded_height appears to iterate over all points whose projective coordinates all have height less than the given bound -- this is not just wrong, it is not well-defined. Similarly, in schemes.projective.projective_point the global height of a point is incorrectly implemented as the max of the heights of its coordinates -- again, not well defined.

CC: @slel @EnderWannabe

Component: number theory

Keywords: projective height

Author: Jieao Song, Frédéric Chapoton

Branch: 71e63df

Reviewer: Alexander Galarraga

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

[8d1h]

comment:2

Hi, I updated my code and added some docs (see the .py file here https://github.com/8d1h/RationalPoints/)

The global height is implemented without log, since this gives the precise value for rational numbers.

The rational point enumeration method uses elimination and is a lot faster than the current available algorithms (see the documented examples).

[mkoeppe]

comment:3

Moving to 9.4, as 9.3 has been released.

[fchapoton]

Branch: public/ticket/31400

[fchapoton]

Commit: 15b37ca

[fchapoton]

comment:5

I have ported the code for global_height from "gh-8d1h" git repo. No idea if this is correct and better than before.

---

New commits:

15b37ca

modify global_height for projective points

[fchapoton]

comment:7

would someone please provide a doctest where the value was wrong before ?

[8d1h]

comment:8

Hi! Sorry I was going to fix this but I didn't quite figure out how the whole trac thing works.

For doctests, I put two of them in my file.

sage: P = ProjectiveSpace(QQ, 2)
sage: P(1/1,2/3,5/8).global_height()
2.77258872223978
sage: F.<u> = NumberField(x^3-5)
sage: P = ProjectiveSpace(F, 2)
sage: P(u,u^2/5,1).global_height()
0.536479304144700

These are the current wrong results: they should be log(24) and 1.07295860828940 respectively.

[sagetrac-git]

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

be63bb3	Merge branch 'public/ticket/31400' in 9.5.b1
8b67f8b	adding doctests

[sagetrac-git]

Changed commit from 15b37ca to 8b67f8b

[fchapoton]

comment:10

Thanks. I have added these doctests.

[fchapoton]

comment:11

Is there any other program that can be used to check these values ?

[8d1h]

comment:12

I guess you could try Magma at http://magma.maths.usyd.edu.au/calc/

P := ProjectiveSpace(Rationals(),2);
p := P ! [1/1,2/3,5/8];
Log(HeightOnAmbient(p:Absolute));

R<x> := PolynomialRing(Integers());
F<u> := NumberField(x^3-5);
P := ProjectiveSpace(F,2);
p := P ! [u,u^2/5,1];
Log(HeightOnAmbient(p:Absolute));

although there are some differences in conventions, hence the Log and :Absolute.

(BTW I was trying to use Sage to check these values for my computation in Macaulay2 in the first place, and it turned out that the function is not correctly implemented...)

[fchapoton]

comment:13

ok, looks good, same results in magma. I would propose to keep the other problem about the iterator for another ticket.

@JohnCremona, @roed314, would you please review ?

[EnderWannabe]

comment:14

The functionality looks good to me.

We have an issue with return types.

sage: P.<x,y,z> = ProjectiveSpace(QQ,2)
sage: Q = P.point([4, 4, 1/30])
sage: type(Q.global_height())
<class 'sage.symbolic.expression.Expression'>

While sometimes we return a real number. Since the function previously returned a real number, we shouldn't change the return type.

[EnderWannabe]

Reviewer: Alexander Galarraga

[fchapoton]

comment:15

so you prefer the inexact answer to the exact answer ?

[EnderWannabe]

comment:16

Since this a function already included in Sage, we can't change it's return type without somehow deprecating the function.

Additionally, it seems we can't return the exact answer in all cases - some answers are still inexact. Having different types returned from the same function with the same parameters is bad practice.

So here I do prefer the inexact answer. It would be nice to return an exact answer, but I don't think it's possible here.

[8d1h]

comment:17

Actually, it is possible to return the exact answer even for general number fields, by implementing an exact version of complex_embedding using QQbar, which would also be useful in general. by using K.embeddings(QQbar).

But I agree that one should be consistent with the return type. Since 2.global_height() returns an inexact value instead of log(2), I think here it should also return the inexact value.

[sagetrac-git]

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

e35065c

now with numeric doctests

[sagetrac-git]

Changed commit from 8b67f8b to e35065c

[fchapoton]

comment:19

ok, here it goes

[sagetrac-git]

Changed commit from e35065c to 71e63df

[sagetrac-git]

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

71e63df

fix doctests

[EnderWannabe]

comment:22

LGTM

[fchapoton]

comment:23

the author should probably be gh-8d1h ? I just copied and adapted the code

We need an "Author full name"..

[fchapoton]

Author: Jieao Song, Frédéric Chapoton

[vbraun]

Changed branch from public/ticket/31400 to 71e63df

[bhutz]

Changed commit from 71e63df to none

[bhutz]

comment:26

I recently came across this bug in the height functions as well as a couple other height bugs. This ticket did fix the the incorrect values I was seeing in global_height().

I'm trying to see if the other issues already have tickets opened too. Was there a ticket opened for the projective points of bounded height iterator? I couldn't find one when I searched trac.

Thanks.

[bhutz]

comment:27

I've created the iterator ticket as #32686