#9337: Add toric divisors
--------------------------------------------+-------------------------------
Reporter: vbraun | Owner: AlexGhitza
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-4.6
Component: algebraic geometry | Keywords:
Author: Volker Braun | Upstream: N/A
Reviewer: Andrey Novoseltsev | Merged:
Work_issues: non-reduced divisor handling |
--------------------------------------------+-------------------------------
Changes (by novoselt):
* work_issues: add detailed comments to cohomology related methods =>
non-reduced divisor handling
Comment:
Awesome!!! I am pretty sure this will be the final patch, although it is
not yet in its final shape ;-)
1. Converting back and forth between `Polyhedron` and `LatticePolytope`
is something we always had to have and it is a bit surprising that nobody
has done it before. Constructing a containing lattice polytope is also
quite useful, but can we change the behaviour a little bit? I propose the
following:
* `is_lattice_polytope()` works exactly as now;
* `lattice_polytope()` raises a `ValueError` exception if the polyhedron
is not compact OR has non-integral vertices (it is not `NotImplemented`
because lattice polytopes must be compact and have only integral vertices
by definition);
* `enveloping_lattice_polytope()` raises a `ValueError` if the
polyhedron is not compact, but otherwise wraps vertices in integral boxes,
as it is done now in `lattice_polytope`. Alternatively, `lattice_polytope`
function may take some argument that will allow it to construct a polytope
which is strictly bigger, e.g. `lattice_polytope(envelope=True)`.
1. Can you please revert changes that this patch makes in `coefficient`
and add the following to its doctest:
{{{
sage: P2 = toric_varieties.P2()
sage: P2.inject_variables()
Defining x, y, z
sage: D = P2.divisor([(1,x), (1,x)])
sage: D
V(x) + V(x)
sage: D.coefficient(x)
1
}}}
Note that the output should be 2, not 1, and while nobody is likely to
enter divisors in such a weird form, it is conceivable that some code will
generate non-reduced divisors.
1. The same issue makes your code in the new `is_integral` function
behave inconsistently:
{{{
sage: D = P2.divisor([(1/2,x), (1/2,x)])
sage: D
1/2*V(x) + 1/2*V(x)
sage: D.is_integral()
False
sage: D.reduce()
sage: D.is_integral()
True
}}}
I would suggest that this function should try to convert the divisor to a
`ZZ`-vector to check if it is integral or not (calling `coefficient` for
every variable will iterate over divisor again and again). Note that
`_vector_` also does not work nicely: if you call `vector(ZZ, D)` before
reducing, you will get an error. The solution is to first create just a
list of zeroes in `_vector_`, accumulate coefficient values in one
iteration through divisor, and then convert this list to a vector. It also
seems to me that the best way to address non-reduced divisors in
`coefficient` is to `return vector(self)[index]` to avoid code
duplication.
1. You should not have added `Group` on line 1024. If it is not clear in
that sentence that this Python-class refers to the divisor-class (and not
the group of these classes), the sentence has to be restructured. Thanks
for catching "::"!
1. Should we maybe make `_sheaf_cohomology_support` a public method? It
seems to me that it is something end-users may care about, at least for
educational purposes.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9337#comment:43>
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 post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
