Add sage.geometry.abc; remove uses of is_Polyhedron, is_LatticePolytope, is_Cone outside of sage.geometry
Closed this issue · 29 comments
$ git grep 'def is_[A-Z]' src/sage/geometry/
src/sage/geometry/cone.py:def is_Cone(x):
src/sage/geometry/fan.py:def is_Fan(x):
src/sage/geometry/lattice_polytope.py:def is_LatticePolytope(x):
src/sage/geometry/lattice_polytope.py:def is_NefPartition(x):
src/sage/geometry/point_collection.pyx:def is_PointCollection(x):
src/sage/geometry/polyhedron/base.py:def is_Polyhedron(X):
src/sage/geometry/toric_lattice.py:def is_ToricLattice(x):
src/sage/geometry/toric_lattice.py:def is_ToricLatticeQuotient(x):
src/sage/geometry/toric_lattice_element.pyx:def is_ToricLatticeElement(x):
In this ticket, we add ABCs LatticePolytope, ConvexRationalPolyhedralCone, Polyhedron.
A small number of uses outside of sage.geometry will need updating. $ git grep 'geometry.*import.*is_[A-Z]' src/sage/
(Part of meta-ticket #32414)
Depends on #32614
Depends on #32649
CC: @kliem @orlitzky @egourgoulhon
Component: refactoring
Author: Matthias Koeppe
Branch/Commit: 44540f5
Reviewer: Michael Orlitzky
Issue created by migration from https://trac.sagemath.org/ticket/32637
Author: Matthias Koeppe
New commits:
502546b | src/sage/geometry/abc.pyx: New |
79a8ba4 | sage.schemes.toric: Remove use of is_Cone |
6e32262 | src/sage/matrix/matrix2.pyx: Remove use of is_Cone |
19aee74 | src/sage/groups/affine_gps/group_element.py: Remove use of is_Polyhedron |
87ad226 | src/sage/manifolds/subsets/pullback.py: Remove use of is_Polyhedron |
Description changed:
---
+++
@@ -12,6 +12,9 @@
src/sage/geometry/toric_lattice_element.pyx:def is_ToricLatticeElement(x):
```
+In this ticket, we add ABCs `LatticePolytope`, `ConvexRationalPolyhedralCone`, `Polyhedron`.
+
+
A small number of uses outside of `sage.geometry` will need updating. `$ git grep 'geometry.*import.*is_[A-Z]' src/sage/`
(Part of meta-ticket #32414)I worry that we're changing is_Foo() to mean "does this object provide the interface defined by abc.Foo?" But then abc.Foo doesn't declare any methods, properties, or data.
On the other hand, you don't want to put the implementation into the base class, because the whole point is to avoid importing the implementations to perform a typecheck. Nor does it make much sense to blindly add a million @abstractmethod def foo(...): pass lines to each ABC. I guess in lieu of an alternative, it has to be OK.
I wonder if something ridiculous like this wouldn't also work as an interim hack:
sage: def is_Polyhedron(C):
....: return "Polyhedron_base" in [c.__name__ for c in C.__class__.__mro__]
You can also check the (string) module name with c.__module__ to avoid name clashes. But chances are, it's slower at runtime.
Replying to @orlitzky:
I worry that we're changing
is_Foo()to mean "does this object provide the interface defined byabc.Foo?" But thenabc.Foodoesn't declare any methods, properties, or data.
As explained in #32414, "each of the new abstract base classes is intended to have a unique direct implementation subclass. Attempting to define new hierarchies of abstract base classes is beyond the scope of this ticket. Also, the new classes do not define any methods.".
Replying to @orlitzky:
I wonder if something ridiculous like this wouldn't also work as an interim hack:
sage: def is_Polyhedron(C): ....: return "Polyhedron_base" in [c.__name__ for c in C.__class__.__mro__]
Yeah, I'm not interested in a solution that looks like this
Replying to @mkoeppe:
As explained in #32414, "each of the new abstract base classes is intended to have a unique direct implementation subclass. Attempting to define new hierarchies of abstract base classes is beyond the scope of this ticket. Also, the new classes do not define any methods.".
I saw that, but unless you plan to live forever and spend most of it monitoring trac, there's no technical means to enforce that restriction.
Sounds like someone wants to write a decorator for these types of classes
Branch pushed to git repo; I updated commit sha1. New commits:
8b9839a | src/sage/manifolds/subsets/pullback.py: Remove more uses of is_Polyhedron |
Replying to @mkoeppe:
Sounds like someone wants to write a decorator for these types of classes
I don't know how to do it with a decorator, but I think a metaclass can ensure that a given class is only subclassed once. As an added bonus, the metaclass can explain why these exist and how they should be used (i.e. that they aren't really abstract base classes).
This example fails (as intended) the second time we subclass:
class TypeCheckWrapper(type):
_subclasses = 0
def __init__(cls, name, bases, clsdict):
if len(cls.mro()) > 2:
TypeCheckWrapper._subclasses += 1
if TypeCheckWrapper._subclasses > 1:
raise TypeError("TypeCheckWrapper with multiple subclasses")
super(TypeCheckWrapper, cls).__init__(name, bases, clsdict)
class Polyhedron(metaclass=TypeCheckWrapper):
pass
print("Defined base class Polyhedron")
class Cone(Polyhedron):
pass
print("Defined class Cone(Polyhedron)")
class Polytope(Polyhedron):
pass
print("Defined class Polytope(Polyhedron)")I think this discussion should go to a separate ticket. Note also that some of the ABCs added in other tickets of #32414 are actually extension classes, and using a metaclass there is probably tricky.
A simple solution could be to just add a doctest for the MRO length. We enforce pretty much everything else with doctests as well.
TESTS::
sage: import sage.geometry.abc
sage: len(sage.geometry.abc.Polyhedron.__subclasses__()) <= 1
True
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
14fd1e5 | src/doc/en/developer/coding_basics.rst: Update discussion of feature tags |
27c53ac | src/sage/features/sagemath.py: Add 'sage.plot' |
180e31d | Merge #30887 |
10e8d63 | sage.features.sagemath: Use JoinFeature when tag is different from the actually tested module |
654d09c | sage.features.sagemath: Change sage_optional_tags to sage_features |
f63a7d0 | src/sage/features/: Move features depending on optional packages to separate files |
4558791 | Merge tag '9.5.beta3' into t/32614/features_and_optional_tags_for_sage_subset_distributions |
f68e511 | Merge #32614 |
18aa26f | src/sage/features/sagemath.py: Add sage.geometry.polyhedron |
ceff8dc | src/sage/geometry/abc.pyx: Add doctests |
How's this?
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9471570 | src/sage/geometry/abc.pyx: Add doctests |
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
9358502 | Merge #32614 |
6457ee9 | src/sage/features/mip_backends.py: Fixup |
eeaa722 | Merge #32614 |
c872d69 | Add description to features |
08248a1 | Fix for doctest failures |
759c88b | sage.doctest, sage.control: Remove unused imports |
15729dc | src/sage/features/mcqd.py: Add doctests |
1d02bd0 | More doctests for coverage |
58572e1 | Merge #32649 |
44540f5 | src/sage/features/sagemath.py: Add doctests |
We actually have a module (sage.cpython.cython_metaclass) that hacks metaclasses into cython using undocumented internals. What could possibly go wrong?
Using doctests is good enough.
Reviewer: Michael Orlitzky
Thanks!