#9337: Add toric divisors
----------------------------------+-----------------------------------------
Reporter: vbraun | Owner: AlexGhitza
Type: enhancement | Status: needs_info
Priority: major | Milestone: sage-4.6
Component: algebraic geometry | Keywords:
Author: Volker Braun | Upstream: N/A
Reviewer: Andrey Novoseltsev | Merged:
Work_issues: |
----------------------------------+-----------------------------------------
Comment(by novoselt):
I have started a little reviewer patch fixing some issues and prettifying
documentation, but it grew up ;-) The patch should go after
`trac_9337_toric_divisors.patch`, I didn't work yet with other files.
Changes:
1. `ToricDivisorGroup` is removed from the global name space. It probably
should not be there, since it is not for other schemes and usually such
things are returned by variety methods. The same argument applies to
`ToricDivisor`, but it seems from your doctests that you did intend to use
it directly. Is it indeed the case? I would prefer to use the form
`X.divisor(...)` which is already there.
1. Only necessary imports are left in the beginning of the file. While it
is a little bit annoying to add more imports when you extend
functionality, I think that keeping imports to minimal helps with speed,
circular references, and clarity.
1. Simplified `_element_constructor_` in toric divisor group, dealing
only with the case when nothing has to be done. There should be no need to
check if two divisor groups are equal - if they are equal, they are (at
least must be) the same object. More sophisticated cases are left to
`ToricDivisor` function to ensure that they always behave in the same way.
I worked on this function as well, hopefully for the good ;-) The original
version has some bugs, e.g.
{{{
sage: P2 = toric_varieties.P2()
sage: ToricDivisor(P2, [(1/2, P2.gen(0), "Extra stuff")])
Divisor 1/2*x
sage: ToricDivisor(P2, [(1/2, P2.gen(0), "Extra stuff")]).parent()
Group of toric ZZ-Weil divisors on 2-d CPR-Fano toric variety covered by 3
affine patches
}}}
(note `ZZ` on the last line). The new one on this example gives
{{{
sage: P2 = toric_varieties.P2()
sage: ToricDivisor(P2, [(1/2, P2.gen(0), "Extra stuff")]).parent()
TypeError: cannot deduce coefficient ring for [(1/2, x, 'Extra stuff')]!
sage: ToricDivisor(P2, [(1/2, P2.gen(0))]).parent()
Group of toric QQ-Weil divisors on 2-d CPR-Fano toric variety covered by 3
affine patches
}}}
1. Some bugs for non-reduced divisors are fixed, e.g.
{{{
sage: P2.divisor([(1,x), (1,x)]).coefficient(x)
1
}}}
while it should be 2.
1. `cl` is changed to `cohomology_class` as for cones, `ch` is now a
synonym for `Chern_caracter` as for toric varieties.
1. Some simplifications to `is_(QQ)_Cartier` code, using the fact that it
is very fast to check Cartier after QQ-Cartier due to your caching of
m-vectors.
1. Some prettifications/clarifications in the documentation, some of them
are due to warnings from Florent's patches at #9128. Note also that you
should not put class documentation into `__init__` docstring, since it
will not show up anywhere.
Remaining questions/issues:
1. Is there really any use for `divisor.m(cone)` for non-QQ-Cartier
divisors? I have a strong desire to check in the beginning of that
function if we are in this situation and if not - raise `ValueError`. But
I may be wrong. And I see that the check involves computing all these m's.
I am also not completely happy about the name `m` which is not very
descriptive, but I don't really have an alternative this time
(`dual_vector` is not a good choice, in my opinion), so let it be for now.
1. I decided once again that I don't like `_repr_` of divisors: `Divisor
x + y` can be interpreted quite naturally as the zero set of `x+y=0`,
while here it means the union of `x=0` and `y=0` (not to mention that in
each case the polynomial may not be well-defined). I guess it stems from
the general divisor behaviour and one should think what to do with it, but
in our case when each "basic" divisor is given by a variable I think we
should change it to `Divisor D_x + D_y`, maybe even dropping `Divisor ` in
front. For general divisors there probably should be something like
`V(...)` around each separate equation, but we don't have to worry about
this now.
1. Some of the examples (like `is_Weil`) are somewhat cumbersome using
"raw" toric varieties. Is there any reason why you didn't want to use
examples from toric varieties library? If there are no varieties on which
it is possible to demonstrate these functions, let's add some!
1. How about making `K` return a canonical cohomology class instead of an
actual divisor?
1. I kind of don't like the idea of complete identification of
divisors/support functions/sheaves... I would rather have methods
`divisor.support_function()` and `divisor.sheaf()`, but of course that
requires creating new classes for these objects, i.e. time...
Once we decide what to do with the above, I can do the changes and
incorporate them into my patch.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9337#comment:24>
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.
