Meta-ticket: Replace is_Class functions, create abstract base classes to enable modularization

[mkoeppe]

git grep 'import.* is_[A-Z]' (or git grep 'import.*is_[A-Z]' | grep -E -v 'is_(Ring|Matrix)[^A-Z]' | grep -v 'sage:') reveals a common pattern in our code: We import a module just to check whether a user-provided object belongs to a class defined by that module.
This pattern is an obstacle to modularization.

The ongoing effort (#12824, https://groups.google.com/g/sage-devel/c/wEzb0awmyN0, #13370, #14329, #15076, #15098, #15333, #18173, #24443, #24525, #27593, #28470, #32347) to replace this pattern by importing the class and isinstance does not help in this regard by itself:
The module providing the class must be installed so that the import does not fail.

This can of course be worked around by try... except around import, as done for example in #32455, but we should avoid cluttering the code with this type of workaround.

Instead we propose to create stub classes (abstract base classes) in sage.structure.element, sage.structure.parent, and/or sage.PAC.KAGE.abc, and make the actual implementation classes subclasses (mostly, unique direct subclasses). This pattern already exists in the Sage code for a few existing classes:

• sage.structure.element defines a base class Matrix (for purposes of the coercion system) and a function is_Matrix (see deprecate some is_*** functions #32347)
• sage.structure.parent defines a base class Set_generic; using it with isinstance has replaced use of the earlier is_Set function (Replace is_Set() usage #24443, remove deprecated is_Set #28470)

We add the following new classes:

• sage.rings.abc #32566/Deprecate is_RealField, is_ComplexField, is_RealDoubleField, is_ComplexDoubleField #32610/sage.rings.abc.{Real,Complex}{Interval,Ball}Field; deprecate is_{Real,Complex}IntervalField #32612: sage.rings.abc defines base class RealDoubleField for sage.rings.real_double.RealDoubleField_class etc.
• Replace is_IntegerModRing by isinstance with new class sage.rings.abc.IntegerModRing #32606
• Add sage.rings.abc.{AlgebraicField,...,NumberField_quadratic}, deprecate is_AlgebraicField, ..., is_CyclotomicField, is_QuadraticField #32660
• sage.rings.abc: Add pAdicRing, pAdicField; deprecate is_pAdic... #32750
• sage.structure.element: Add abc Expression; deprecate is_Expression etc. #32638/Deprecate is_[Callable]SymbolicExpressionRing, remove use of is_Symbolic{Equation,Variable}, is_CallableSymbolicExpression #32665: in sage.structure.element, a class Expression
  • the existing class sage.symbolic.expression.Expression will be its only subclass
  • deprecate is_Expression, is_SymbolicEquation, is_CallableSymbolicExpression, is_SymbolicVariable; replace uses by isinstance(x, Expression) and a method call (may need a new method is_callable)
• Add sage.geometry.abc; remove uses of is_Polyhedron, is_LatticePolytope, is_Cone outside of sage.geometry #32637: in sage.geometry.abc, a class Polyhedron
  • Polyhedron_base will be its only direct subclass
• Add ABCs CommutativePolynomial, MPolynomial_libsingular, InfinitePolynomial; deprecate is_Polynomial, is_MPolynomial #32709
• Deprecate sage.interfaces is_...Element functions #34804
• Deprecate is_FiniteField etc., make sage.rings.finite_rings a namespace package #32664
• New ABC sage.structure.element.NumberFieldElement, deprecate is_NumberFieldElement #34931
• deprecate is_RealNumber, is_AlgebraicNumber, is_RealIntervalFieldElement, ... #24525

Technical limitation: If a sage.PAC.KAGE.abc module is intended to provide the new classes, then sage.PAC.KAGE must be prepared to become a native namespace package. (See Meta-ticket #32501: Clear out __init__.py files in preparation for namespace packages)

As of this ticket, each of the new abstract base classes is intended to have a unique direct implementation subclass. #32637 adds doctests to document and enforce this design. 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.

Tickets not needed for modularization, but removing is_ functions in favor of isinstance for uniformity of the codebase:

• Deprecate is_Algebra, is_CommutativeAlgebra, is_FreeAlgebra, is_QuaternionAlgebra, is_SymmetricFunctionAlgebra #32738
• Deprecate is_Polyhedron, is_Cone, is_LatticePolytope #34307
• sage.modular.hecke: Deprecate is_Hecke... functions #37895
• Deprecate is_FreeAlgebra, is_QuaternionAlgebra, is_SymmetricFunctionAlgebra #37896
• Deprecate is_Monoid, is_FreeMonoid, is_FreeAbelianMonoid #37897
• Deprecate is_AbelianGroup, is_DualAbelianGroup, is_MatrixGroup #37898
• Deprecate is_Category, is_Endset, is_Homset, is_Parent, is_RingHomset, is_SimplicialComplexHomset #37922
• Deprecate is_FGP_Module, is_FilteredVectorSpace, is_FreeQuadraticModule, is_FreeModule, is_FreeModuleHomspace, is_MatrixSpace, is_Module, is_VectorSpace, is_VectorSpaceHomspace #37924
• sage.schemes: Deprecate is_... functions #38022
• sage.combinat.finite_state_machine: Deprecate is_... functions #38032
• sage.modular: Deprecate is_... functions #38035
• Deprecate is_Element, ... #38077
• Deprecate is_Map, is_...Morphism #38103
• Deprecate is_NumberFieldOrder, is_AbsoluteNumberField, is_RelativeNumberField, is_NumberFieldIdeal, is_NumberFieldFractionalIdeal, is_NumberFieldFractionalIdeal_rel #38124
• Deprecate is_MutablePoset #38125
• Deprecate is_Fan, is_NefPartition, is_PointCollection, is_ToricLattice, is_ToricLatticeElement, is_ToricLatticeQuotient #38126
• Deprecate is_AlgebraicNumber, is_AlgebraicReal, is_ComplexDoubleElement, is_ComplexIntervalFieldElement, is_ComplexNumber, is_FractionField, is_FractionFieldElement, is_Integer, is_IntegerMod, is_IntegerRing, is_Rational, is_RationalField, is_RealDoubleElement, is_RealIntervalField, is_RealIntervalFieldElement, is_RealNumber #38128
• Deprecate is_FreeAlgebraQuotientElement, ... #38184
• Deprecate is_Ideal, is_LaurentSeries, is_MPolynomialIdeal, is_MPolynomialRing, is_MPowerSeries, is_PolynomialQuotientRing, is_PolynomialRing, is_PolynomialSequence, is_PowerSeries, is_QuotientRing #38266
• to be done: see https://groups.google.com/g/sage-devel/c/6MJHDicPYWs/m/RBxTi4TGAwAJ

Functions that look like is_Class:

• Rename is_GlobalGenus, Genus_Symbol_p_adic_ring #38287
• Rename is_NumberFieldHomsetCodomain #38297

Related:

• QuaternionAlgebra: Unify ABC and factory #38228
• FreeAlgebra: Unify ABC and factory #38240

Part of:

• Meta-ticket: Modularize sagelib into separate distributions (pip-installable packages) sagemath-... for Sage 10.x #29705

CC: @tscrim @fchapoton @kliem

Component: refactoring

Issue created by migration from https://trac.sagemath.org/ticket/32414

[kliem]

comment:1

I find this in general bothering that there are tons of modules etc. being imported (even at startup), just because some isinstance check.

• The best I can come up with for the second is the following:

_Polyhedron_base = None

class _dummy:
    pass

def is_polyhedron(P):
    global _Polyhedron_base
    if _Polyhedron_base is not None:
        return isinstance(P, _Polyhedron_base)
    try:
        from sage.geometry.polyhedron.base import Polyhedron_base
        _Polyhedron_base = Polyhedron_base
        return isinstance(P, Polyhedron_base)
    except ImportError:
        _Polyhedron_base = _dummy
        return False

Apparently its a slow down of about 50 percent, when the import is found, otherwise much more.

• The first thing is even faster (isinstance(P, Polyhedron_containment_check) is about 75 percent of isinstance(P, Polyhedron_base)), but I don't know how much another stupid inheritance slows down the actual class.

  I would prefer a unique name scheme that can be used throughout sage. _base is already in use. Could as well be the original class name with a leading underscore to discourage people to use it for anything else, e.g. _Polyhedron_base, _Expression.

[mkoeppe]

comment:6

Thanks for these thoughts and experiments. I also prefer the approach using new base classes; and also I have learned more about the multiyear effort to get rid of is_... methods. I have updated the ticket description accordingly

[mkoeppe]

Branch: u/mkoeppe/replace_some_is_____functions_by_isinstance_with_new_base_classes_to_enable_modularization

[mkoeppe]

comment:10

Here's an attempt for Polyhedron. Still need to do some trick to make the constructor's docstring available

---

New commits:

4f0345b	sage.structure.element.Polyhedron: New base class / constructor
f7f15a7	is_Polyhedron: Deprecate

[mkoeppe]

Commit: f7f15a7

[mkoeppe]

Author: Matthias Koeppe, ...

[mkoeppe]

comment:12

For getting the docstring I have tried to make __doc__ a property, but this approach does not seem to work for Cython classes...

[sagetrac-git]

Changed commit from f7f15a7 to 9552be9

[sagetrac-git]

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

9552be9

WIP: Try to make `__doc__` a class property

[mkoeppe]

comment:14

I think I'll need some help from Cython experts here...

[tscrim]

comment:15

I think you're running into the fact that the Python special double underscore methods are handled in a special way. I don't know too much about how that's done, but that that is my recollection.

[mkoeppe]

comment:16

A different approach would be to keep the base class / constructor in sage.geometry.polyhedron.constructor (similar to the branch of #26366) and to assign this module to the distribution sagemath-categories.

[mkoeppe]

Changed commit from 9552be9 to none

[mkoeppe]

Changed author from Matthias Koeppe, ... to none

[mkoeppe]

Changed branch from u/mkoeppe/replace_some_is_____functions_by_isinstance_with_new_base_classes_to_enable_modularization to none