Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
sagetrac-gjorgenson mannequin opened this issue Nov 24, 2014 · 15 comments
Closed

Comments

@sagetrac-gjorgenson
Copy link
Mannequin

sagetrac-gjorgenson mannequin commented Nov 24, 2014

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.

CC: @bhutz

Component: algebraic geometry

Author: Grayson Jorgenson

Branch: 63b6422

Reviewer: Ben Hutz

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

@sagetrac-gjorgenson sagetrac-gjorgenson mannequin added this to the sage-6.5 milestone Nov 24, 2014
@sagetrac-gjorgenson
Copy link
Mannequin Author

sagetrac-gjorgenson mannequin commented Nov 24, 2014

Branch: u/gjorgenson/ticket/17386

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 28, 2014

Commit: 456c721

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 28, 2014

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

456c721Added enumeration functionality for affine space over number fields.

@bhutz
Copy link

bhutz commented Dec 18, 2014

Reviewer: Ben Hutz

@bhutz
Copy link

bhutz commented Dec 18, 2014

comment:4

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 27, 2015

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 27, 2015

Changed commit from 456c721 to 003884c

@bhutz
Copy link

bhutz commented Jan 29, 2015

comment:7

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 3, 2015

Changed commit from 003884c to f786719

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 3, 2015

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 3, 2015

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

63b642217386: removed uneeded references

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 3, 2015

Changed commit from f786719 to 63b6422

@bhutz
Copy link

bhutz commented Feb 3, 2015

comment:11

Ok. this looks good now.

@vbraun
Copy link
Member

vbraun commented Feb 18, 2015

Changed branch from u/gjorgenson/ticket/17386 to 63b6422

@vbraun vbraun closed this as completed in 3518eb6 Feb 18, 2015
@sagetrac-gjorgenson
Copy link
Mannequin Author

sagetrac-gjorgenson mannequin commented Feb 20, 2015

Changed commit from 63b6422 to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants