#9337: Add toric divisors
----------------------------------+-----------------------------------------
Reporter: vbraun | Owner: AlexGhitza
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-5.0
Component: algebraic geometry | Keywords:
Author: Volker Braun | Upstream: N/A
Reviewer: Andrey Novoseltsev | Merged:
Work_issues: |
----------------------------------+-----------------------------------------
Changes (by novoselt):
* status: new => needs_work
* reviewer: => Andrey Novoseltsev
Comment:
I went over the first patch and it looks great except for the following
minor points:
* The name of the argument in this function is confusing. Since such
functions take anything as input, I vote for something neutral like `x`:
{{{
def is_DivisorGroup(divisor):
...
}}}
* In `_element_constructor_(self, x, check=True, reduce=True)` the last
line is
{{{
return Divisor_generic([(self.base_ring()(1), x)], check=False,
reduce=False, parent=self)
}}}
Why in this case `check` and `reduce` are set to `False` instead of using
the passed values?
* As I understood the documentation of `UniqueRepresentation`, derived
classes should not have default values in `__init__`, because it leads to
this:
{{{
sage: from sage.structure.formal_sum import FormalSums_generic
sage: FormalSums_generic(ZZ) is FormalSums_generic()
False
}}}
Note, however,
{{{
sage: FormalSums(ZZ) is FormalSums()
True
}}}
since each layer repeats default values and the default in the actual
class is never used (unless you do it directly as I have done it above).
This brings me to the last comment:
* Is there any need in factory functions now that they just call the
class? Before they were necessary since they were providing
caching/uniqueness, but your new approach is cleaner and more unified
(leading to "unique unique representation" ;-)).
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9337#comment:9>
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.
