#19635: Enumeration functionality for products of projective spaces over fields
and
finite fields
-------------------------------------+-------------------------------------
Reporter: gjorgenson | Owner:
Type: enhancement | Status: needs_work
Priority: minor | Milestone: sage-7.1
Component: algebraic | Resolution:
geometry | Merged in:
Keywords: | Reviewers: Ben Hutz
Authors: Grayson Jorgenson | Work issues:
Report Upstream: N/A | Commit:
Branch: | 449d0738bfd0d9e85deea38f5ff94cdf1356e745
u/gjorgenson/ticket/19635 | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Changes (by bhutz):
* status: needs_review => needs_work
* reviewer: => Ben Hutz
Comment:
A few minors things and a few larger things. I think before I do
functionality testing some of these coding improvements should be made.
Minor
- You are changing the example in the ring class to give a field point.
Perhaps you should change the ring instead of the output so that it is
still a ring example
{{{
- <class
'sage.schemes.product_projective.point.ProductProjectiveSpaces_point_ring'>
+ <class
'sage.schemes.product_projective.point.ProductProjectiveSpaces_point_field'>
}}}
- no ending punctuation on errors
- in both of your brute force point searches you don't need the
intermediate variable. You can just have:
{{{
for P in X.ambient_space().rational_points():
}}}
- remove references to self in doc strings. Instead say things like, this
point, or this space.
- there are a number of spacing issues
- between lists of polys, such as [x^2-y^2, z^2-2*w^2]
- between function parameters, such as X.affine_patch(I, True)
- between coordinates of points
- references need an underscore, [Doyle-Krumm]_
points function
- you should mention that this is computing all points for dimension 0
schemes regardless of base ring.
- output: should be "all points with height at most the bound are
returned"
- you might need to .sort() your doctests so that they don't depend on the
machine
Major
- with 7.1.beta1 there is a depracation in one of the function you use
which causes a number of doctest failures, you should update to the new
function.
{{{
sage -t --warn-long 56.2 schemes/generic/algebraic_scheme.py # 1 doctest
failed
sage -t --warn-long 56.2 schemes/product_projective/wehlerK3.py # 1
doctest failed
sage -t --warn-long 56.2 schemes/projective/projective_space.py # 1
doctest failed
sage -t --warn-long 56.2 schemes/product_projective/space.py # 1 doctest
failed
sage -t --warn-long 56.2 schemes/product_projective/morphism.py # 1
doctest failed
sage -t --warn-long 56.2 schemes/product_projective/point.py # 1 doctest
failed
sage -t --warn-long 56.2 schemes/product_projective/homset.py # 1 doctest
failed
}}}
points function
- your dimension check doesn't actually work in all cases. I'd recommend
using #19991 as a dependency and switching to that dimension call.
- You seem to be going to a lot of trouble to generate possible indices
for affine patches. Here is something shorter
{{{
dims = [n+1 for n in X.ambient_space().dimension_relative_components()]
for I in xmrange(dims):
}}}
You should be able to improve the indices creator in
points_of_bounded_height and rational_points as well
- You are returning points in the ambient space instead of the scheme. You
should just have
{{{
points.add(phi(PP))
}}}
- hash() - for field, finite_field are exactly the same as for ring. So
you don't need these functions as they will inherit, but add their
examples to ring.
- The example changes in SchemeMorphism_polynomial_projective_space_field.
why did they change? Are these are sorting problem?
- in rationalpoints()
I don't like that you are generating all the points in all the projective
spaces and then generating all possible indices. We can do this more
memory efficiently; something like:
{{{
P = ProductProjectiveSpaces([1,1,1,2],GF(2))
iters = [ iter(T) for T in P ]
L=[]
for x in iters:
L.append(next(x)) # put at zero
points=[P(L)]
j = 0
while j < P.num_components():
try:
L[j] = next(iters[j])
points.append(copy(P(L)))
j = 0
except StopIteration:
iters[j] = iter(P[j]) # reset
L[j] = next(iters[j]) # put at zero
j += 1
}}}
speaking of which. I'd like to see this as an _iter_ function in addition
to a rational_points function.
--
Ticket URL: <http://trac.sagemath.org/ticket/19635#comment:5>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at https://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
