#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: |
Description (last modified by )
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
- Cc slelievre added
- Description modified (diff)
comment:2 Changed 18 months ago by
comment:3 Changed 15 months ago by
- 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
- Milestone changed from sage-9.4 to sage-9.5
comment:5 Changed 11 months ago by
- 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:
15b37ca | modify global_height for projective points
|
comment:6 Changed 11 months ago by
- Cc gh-EnderWannabe added
comment:7 Changed 11 months ago by
would someone please provide a doctest where the value was wrong before ?
comment:8 Changed 11 months ago by
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
- Commit changed from 15b37cacc178272923528ea816ced3fdf2a49d57 to 8b67f8b028c5ceb68514dd613ca3b088b679d44e
comment:10 Changed 11 months ago by
Thanks. I have added these doctests.
comment:11 Changed 11 months ago by
Is there any other program that can be used to check these values ?
comment:12 Changed 11 months ago by
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
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
- 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
so you prefer the inexact answer to the exact answer ?
comment:16 Changed 11 months ago by
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
Actually, it is possible to return the exact answer even for general number fields, by implementing an exact version of by using complex_embedding
using QQbar
, which would also be useful in general.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.
comment:18 Changed 11 months ago by
- Commit changed from 8b67f8b028c5ceb68514dd613ca3b088b679d44e to e35065cf3af39d883e1d25ba741f3505287039a4
Branch pushed to git repo; I updated commit sha1. New commits:
e35065c | now with numeric doctests
|
comment:19 Changed 11 months ago by
ok, here it goes
comment:20 Changed 11 months ago by
- Status changed from needs_work to needs_review
comment:21 Changed 11 months ago by
- Commit changed from e35065cf3af39d883e1d25ba741f3505287039a4 to 71e63dfeb71fea4fa6dad05e98a31aec16eba058
Branch pushed to git repo; I updated commit sha1. New commits:
71e63df | fix doctests
|
comment:23 Changed 11 months ago by
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
comment:25 Changed 10 months ago by
- 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
- 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
I've created the iterator ticket as #32686
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).