#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: |
----------------------------------+-----------------------------------------
Changes (by novoselt):
* status: needs_work => needs_info
* work_issues: subsequent patch rebasement =>
Comment:
I couldn't believe my eyes for a long time while I was starring at the
part of divisor classes patch related to `polyhedra.py` module, but once I
opened the existing library version it made sense ;-)
Changes in the reviewer patch (which should be applied on top of all
others):
* I renamed `test` to `region` as an option for `cone._contains`, `test`
seems to me somewhat confusing, I would think that it is used as a boolean
value, not a string. I also removed underscores from acceptable input
since they are not used as Python identifiers and then I think it is
better to make them more human.
* I renamed `divisor.P()` to `divisor.polytope()`, hope it is OK ;-)
* I renamed `gale_transform` to `Gale_transform` for fans in the spirit
of all other names in our files. This breaks backward compatibility with
the existing version of Sage, but I think that it is so new that it is OK,
especially since it is kind of a bug ;-)
* I added `Mori_cone` method, `Mori_vectors` now returs its rays (do you
think we still need it?). What exactly "Mori vectors" refer to? Generators
of the cone, or any point inside this cone as well? What about non-
simplicial cases, when generators are not unique? `Kaehler_cone.dual()`
gives something pretty horrible, so it should not be used by end-users
until we make it behave better, but it is useful to construct `Mori_cone`,
including non-simplicial cases.
* Some functions now return tuples instead of lists. I think that for
cached values it is important, while for others it is good for consistency
(and in case we decide to make them cached as well).
* Some code optimization/bug fixing (Hal Schenck pointed at some of them
while working on examples).
* Some documentation prettification, e.g. while it is not necessary to
give keyword arguments, I like to write `Fan(cones=[...], rays=[...])` in
doctests since it makes it more clear for the reader which list is what
and where are their boundaries.
Food for thoughts:
1. Do you have any doubts about using the name `Kaehler_cone` for the
closure of the actual Kaehler cone? (I am OK with it, just want to make
sure that you have thought about it as well).
1. Is it really necessary to set `_fan` attribute of divisors? It seems
to me that if you need it once in a function, it is not that much work to
get to the fan though the scheme attribute, while when you need it often
you can get it once and name just `fan` which is even more convenient.
1. I really don't like that "cohomology" is used to refer to a vector of
integers. I would expect it to return actual cohomology groups (like
homology method of simplicial complexes does). How about renaming all
relevant method and their documentation and call it something else?
"D.h_vector()" comes to mind, or maybe even "D.h()" to get a vector and
"D.h(2)" to get the dimension of the given degree only.
1. I think all cohomology-related functions (including helpers) should
have explanations of / references to used algorithms, either in the
documentation, or in the form of comments. The second variant may be even
more appropriate.
1. How about changing "The toric QQ-divisor class group" to "The toric
rational divisor class group" in `_repr_`? Since everything else is
written out explicitly, it seems to me that it is better without "QQ" (but
I am OK with the current version, if you prefer it).
1. I think we must have a space after comma in divisor classes
representation, i.e. have in `_repr_` something like `return "Divisor
class %s" % list(self)`.
1. I think that we should derive `ToricRationalDivisorClass` from vectors
in the same way as it is done for toric lattice elements, because
overriding `_new_c` eliminates the necessity to repeat every single
arithmetic function, which involves multiple unnecessary conversions.
1. How about renaming `divisor_class_group` to just `class_group`? Or
better `rational_class_group` to emphasize tensoring with QQ and with a
hope to implement eventually `class_group` with potential torsion.
I am happy to do any/all (except for (4)) of the changes above as a part
of the reviewer patch, once we agree on them.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9337#comment:34>
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.
