Opened 18 months ago

Closed 10 months ago

Last modified 10 months ago

#31400 closed defect (fixed)

Serious errors in heights of projective points over number fields

Reported by: cremona Owned by:
Priority: major Milestone: sage-9.5
Component: number theory Keywords: projective height
Cc: slelievre, gh-EnderWannabe Merged in:
Authors: Jieao Song, Frédéric Chapoton Reviewers: Alexander Galarraga
Report Upstream: N/A Work issues:
Branch: 71e63df (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by slelievre)

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.

Change History (27)

comment:1 Changed 18 months ago by slelievre

  • Cc slelievre added
  • Description modified (diff)

comment:2 Changed 18 months ago by gh-8d1h

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).

comment:3 Changed 15 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Moving to 9.4, as 9.3 has been released.

comment:4 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:5 Changed 11 months ago by chapoton

  • Branch set to public/ticket/31400
  • Commit set to 15b37cacc178272923528ea816ced3fdf2a49d57
  • Status changed from new to needs_review

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:

15b37camodify global_height for projective points

comment:6 Changed 11 months ago by gh-EnderWannabe

  • Cc gh-EnderWannabe added

comment:7 Changed 11 months ago by chapoton

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

comment:8 Changed 11 months ago by gh-8d1h

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.

comment:9 Changed 11 months ago by git

  • Commit changed from 15b37cacc178272923528ea816ced3fdf2a49d57 to 8b67f8b028c5ceb68514dd613ca3b088b679d44e

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

be63bb3Merge branch 'public/ticket/31400' in 9.5.b1
8b67f8badding doctests

comment:10 Changed 11 months ago by chapoton

Thanks. I have added these doctests.

comment:11 Changed 11 months ago by chapoton

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

comment:12 Changed 11 months ago by gh-8d1h

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...)

comment:13 Changed 11 months ago by chapoton

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

@cremona, @roed, would you please review ?

comment:14 Changed 11 months ago by gh-EnderWannabe

  • Reviewers set to Alexander Galarraga
  • Status changed from needs_review to needs_work

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.

comment:15 Changed 11 months ago by chapoton

so you prefer the inexact answer to the exact answer ?

comment:16 Changed 11 months ago by gh-EnderWannabe

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.

comment:17 Changed 11 months ago by gh-8d1h

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.

Last edited 11 months ago by gh-8d1h (previous) (diff)

comment:18 Changed 11 months ago by git

  • Commit changed from 8b67f8b028c5ceb68514dd613ca3b088b679d44e to e35065cf3af39d883e1d25ba741f3505287039a4

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

e35065cnow with numeric doctests

comment:19 Changed 11 months ago by chapoton

ok, here it goes

comment:20 Changed 11 months ago by chapoton

  • Status changed from needs_work to needs_review

comment:21 Changed 11 months ago by git

  • Commit changed from e35065cf3af39d883e1d25ba741f3505287039a4 to 71e63dfeb71fea4fa6dad05e98a31aec16eba058

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

71e63dffix doctests

comment:22 Changed 11 months ago by gh-EnderWannabe

  • Status changed from needs_review to positive_review

LGTM

comment:23 Changed 11 months ago by chapoton

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

We need an "Author full name"..

comment:24 Changed 11 months ago by chapoton

  • Authors set to Jieao Song, Frédéric Chapoton

comment:25 Changed 10 months ago by vbraun

  • Branch changed from public/ticket/31400 to 71e63dfeb71fea4fa6dad05e98a31aec16eba058
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:26 Changed 10 months ago by bhutz

  • Commit 71e63dfeb71fea4fa6dad05e98a31aec16eba058 deleted

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.

comment:27 Changed 10 months ago by bhutz

I've created the iterator ticket as #32686

Note: See TracTickets for help on using tickets.