Add sage.geometry.abc; remove uses of is_Polyhedron, is_LatticePolytope, is_Cone outside of sage.geometry

[mkoeppe]

$ 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

[mkoeppe]

Branch: u/mkoeppe/add_sage_geometry_abc__deprecate_is_polyhedron__is_latticepolytope__is_cone_____

[mkoeppe]

Author: Matthias Koeppe

[mkoeppe]

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

[mkoeppe]

Commit: 87ad226

[mkoeppe]

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)

[orlitzky]

comment:5

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.

[mkoeppe]

comment:6

Replying to @orlitzky:

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.

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.".

[mkoeppe]

comment:7

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

[orlitzky]

comment:8

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.

[mkoeppe]

comment:9

Sounds like someone wants to write a decorator for these types of classes

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

8b9839a

src/sage/manifolds/subsets/pullback.py: Remove more uses of is_Polyhedron

[sagetrac-git]

Changed commit from 87ad226 to 8b9839a

[orlitzky]

comment:11

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)")

[mkoeppe]

comment:12

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.

[mkoeppe]

comment:13

A simple solution could be to just add a doctest for the MRO length. We enforce pretty much everything else with doctests as well.