#8986: Add support for convex rational polyhedral cones
----------------------------------+-----------------------------------------
Reporter: novoselt | Owner: mhampton
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-4.4.4
Component: geometry | Keywords:
Author: Andrey Novoseltsev | Upstream: N/A
Reviewer: Volker Braun | Merged:
Work_issues: |
----------------------------------+-----------------------------------------
Changes (by vbraun):
* status: needs_review => needs_work
* reviewer: => Volker Braun
Comment:
Looks good! I have two comments:
== Strict subcone ==
I find `ConvexRationalPolyhedralCone.strict_subcone()` confusing: It does
return a quotient cone, not a subcone. Maybe we can call it
`strict_quotient()`?
== point in cone ==
`IntegralRayCollection.__contains__(ray)` tests whether `ray` is a
reference to one of the generating rays. But this is then inherited by
`ConvexRationalPolyhedralCone` Naively, I would have expected that it
tests whether something is in the cone:
{{{
sage: octant = Cone([(1,0,0), (0,1,0), (0,0,1)])
sage: N = octant.lattice()
sage: n = N(1,1,1)
sage: n.set_immutable()
sage: n in octant
False
sage: (1,0,0) in octant
False
}}}
Similarly there are issues with the immutablity as shown in the doctest.
I would suggest the following:
1) get rid of `IntegralRayCollection.__contains__(ray)`. If you need to
test membership, its easy enough to search `self.rays()` or
`self.rays_set()`:
{{{
sage: octant = Cone([(1,0,0), (0,1,0), (0,0,1)])
sage: N = octant.lattice()
sage: N(1,0,0) in octant.rays() # no problem with immutability
True
sage: n = N(1,0,0)
sage: n.set_immutable()
sage: n in octant.ray_set() # slightly more efficient and its clear why n
must be immutable
True
}}}
2) tighten the rules for comparison of N/M-lattice objects:
{{{
sage: M = N.dual()
sage: M(1,0,0) == N(1,0,0) # this should raise an error
False
sage: (1,0,0) == N(1,0,0) # should probably return true
False
sage: M(1,0,0) == (1,0,0) # should probably return true as well
False
sage: (1,0,0) == (1,0,0) # works as expected
True
}}}
This would fix the following:
{{{
sage: (1,0,0) in octant.rays() # this should return true
False
sage: M(1,0,0) in octant.rays() # this should definitely raise an error
False
}}}
3) add methods `contains(n)` to `ConvexRationalPolyhedralCone` to test
whether a N-lattice point is contained in the cone, e.g. (untested)
{{{
def contains(self, *Nlist):
"""
Returns whether the cone contains the N-lattice points ``*Nlist``.
EXAMPLES::
sage: cone = Cone([[1,0],[0,1]])
sage: n = cone.ray(0) + cone.ray(1)
sage: cone.contains(n)
True
sage: cone.contains( N(1,1), N(-1,1) )
False
sage: cone.contains([ N(1,1), N(-1,1) ])
False
"""
pts = flatten(Nlist)
assert all(n in self._lattice() for n in pts), 'The points
'+str(pts)+' must be in the N-lattice.'
return all( self.polyhedron().contains(n) for n in pts )
}}}
4) `ConvexRationalPolyhedralCone.__contains__()` just calls `contains()`.
This is syntactic sugar, but `__` methods alone don't show up in the
documentation.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/8986#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 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.
