Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#17386 closed enhancement (fixed)

Enumerate points of bounded height in projective/affine space over number fields

Reported by: gjorgenson Owned by:
Priority: minor Milestone: sage-6.6
Component: algebraic geometry Keywords:
Cc: bhutz Merged in:
Authors: Grayson Jorgenson Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: 63b6422 (Commits) Commit:
Dependencies: Stopgaps:

Description

Add functionality to iterate over points of bounded height in projective/affine space over a number field. Also add functionality to list the points of height less than a given bound on a scheme defined over a number field.

Change History (13)

comment:1 Changed 4 years ago by gjorgenson

  • Branch set to u/gjorgenson/ticket/17386
  • Created changed from 11/24/14 05:26:20 to 11/24/14 05:26:20
  • Modified changed from 11/24/14 05:26:20 to 11/24/14 05:26:20

comment:2 Changed 4 years ago by git

  • Commit set to 456c7213880388ca6311348c55a798cf5a9b1187

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

456c721Added enumeration functionality for affine space over number fields.

comment:3 Changed 4 years ago by gjorgenson

  • Status changed from new to needs_review

comment:4 Changed 4 years ago by bhutz

  • Reviewers set to Ben Hutz
  • Status changed from needs_review to needs_work

Here is a first pass at reviewing this.

First you need to be careful with the difference between relative height and absolute height. The function K.elements_of_bounded_height() is returning with respect to a bound of relative height (as its documentation says). I would think most users would be interested in absolute height rational points for schemes. So you'll need to fix the functionality here, but also fix the documentation to specify which height in all appropriate places.

Also, specify whether the bound is <= or <

numberField is not a good variable name as CamelCase is reserved for class names.

In projective_homset.py put the imports near where they are needed so you only import what you need (this is done correctly in affine_homset.py)

Some examples that I expected to work that didn't

u = QQ['u'].0
K.<v> = NumberField(u^2 + 3)
A.<x,y> = ProjectiveSpace(K,1)
X=A.subscheme(x-y)
X.rational_points(3)

same for affine

from sage.schemes.projective.projective_rational_point import enum_projective_number_field
enum_projective_number_field(X,3)
from sage.schemes.affine.affine_rational_point import enum_affine_number_field
enum_affine_number_field(X,3)

I think the issues with the enum_ functions is whether the input should be the scheme X or the point set X(K). Clarify which in the documentation for the functions. However, the X.rational_points ones seem to actually be failing.

comment:5 Changed 4 years ago by git

  • Commit changed from 456c7213880388ca6311348c55a798cf5a9b1187 to 003884c155f7fc53bd649617fc5c983e545aefbd

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

9eb56a317386: Implemented some of the changes discussed by the review.
9e610df17386: Merge branch 'master' into ticket/17386
003884c17386: finished implementing changes from first review

comment:6 Changed 4 years ago by gjorgenson

  • Status changed from needs_work to needs_review

comment:7 Changed 4 years ago by bhutz

  • Status changed from needs_review to needs_work

Looks like we are almost there with this one. The functionality all is in working order. But, I came across a few minor things to fix.

affine_rational_point example line 168 is a long test so add -> # long time probably makes more sense to make the bound smaller and get the test < 1s

affine_space -> line 777 not needed, but move the comment to 779

affine_space -> points_of_bounded_height -> add comment that this is using the Krumm-Doyle algorithm for number fields (get the citation from number fields where they implemented it).

algebraic_scheme -> line 1502 no longer need is_RationalField(F)

projective_space -> points_of_bounded_height -> add comment that this is using the Krumm-Doyle algorithm for number fields (get the citation from number fields where they implemented it).

projective_space -> line 94 not needed -> move comment to 951

There are a few places where the lines in the docs are too long. You need to wrap those. Such as line 748 in affine_space.py

Another bad example:

u = QQ['u'].0
K.<v> = NumberField(u^2 + 3)
A.<x,y> = ProjectiveSpace(K,1)
X=A.subscheme(x-y)
X.rational_points(3)
from sage.schemes.affine.affine_rational_point import enum_affine_number_field
enum_affine_number_field(X,3)

oops...X is projective and we called enum_affine successfully!! Need a check in those functions to make sure the ambient space is Affine or Projective

comment:8 Changed 4 years ago by git

  • Commit changed from 003884c155f7fc53bd649617fc5c983e545aefbd to f786719b1183eb412e2898deb002b9e3cf3603d4

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

f78671917386: Implemented changes from second review. In addition to these changes, also added checks in

comment:9 Changed 4 years ago by gjorgenson

  • Status changed from needs_work to needs_review

comment:10 Changed 4 years ago by git

  • Commit changed from f786719b1183eb412e2898deb002b9e3cf3603d4 to 63b6422759a26bc2393a952b186731296e14fe48

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

63b642217386: removed uneeded references

comment:11 Changed 4 years ago by bhutz

  • Status changed from needs_review to positive_review

Ok. this looks good now.

comment:12 Changed 3 years ago by vbraun

  • Branch changed from u/gjorgenson/ticket/17386 to 63b6422759a26bc2393a952b186731296e14fe48
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 Changed 3 years ago by gjorgenson

  • Commit 63b6422759a26bc2393a952b186731296e14fe48 deleted
  • Milestone changed from sage-6.5 to sage-6.6
Note: See TracTickets for help on using tickets.