sage.rings.abc

[mkoeppe]

This new module will provide abstract base classes for the purpose of isinstance and issubclass testing. This helps with modularization (#32432).

In this ticket, we add RealField, RealDoubleField, ComplexField, ComplexDoubleField, which are in 1:1 correspondence to the implementation classes RealField_class, RealDoubleField_class, ComplexField_class, ComplexDoubleField_class.

To illustrate the use of these classes, we convert some uses of is_... functions to isinstance with the new base classes.

Related:

• https://groups.google.com/g/sage-devel/c/T0A4JCOg9DY/m/q6WKsI1yBAAJ (Sep 2021)

Part of meta-ticket #32414.

CC: @tscrim @kliem @videlec @nbruin

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 6e2c3c4

Reviewer: Jonathan Kliem

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

[mkoeppe]

Branch: u/mkoeppe/sage_rings_abc

[sagetrac-git]

Commit: e1bb944

[sagetrac-git]

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

99f4f10	sage.rings.real_double.RealDoubleField_class: Inherit through a new class sage.rings.abc.RealDoubleField
29871d0	ComplexField_class, ComplexDoubleField: Inherit through new classes sage.rings.abc.*
e1bb944	src/sage/rings/polynomial/polynomial_element.pyx: Replace use of is_RealField by isinstance(sage.rings.abc.RealField) etc.

[mkoeppe]

Author: Matthias Koeppe

[tscrim]

comment:4

I feel like this might go back towards the Ring subclass of Parent that we had before and have been trying to get away from by using the categories. Is there a specific reason why to put this back? I know the membership testing is slower, but is there another reason? Is there a specific membership test that is a major bottleneck?

[sagetrac-git]

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

6e2c3c4

src/sage/rings/abc.pyx: Add class docstrings

[sagetrac-git]

Changed commit from e1bb944 to 6e2c3c4

[mkoeppe]

comment:7

Replying to @tscrim:

I feel like this might go back towards the Ring subclass of Parent that we had before and have been trying to get away from by using the categories. Is there a specific reason why to put this back?

In #32414 I now explain that this is expressly not done in order to create a new hierarchy according to mathematical properties.

It's really just to make sure that code can test whether an object belongs to a class even if the implementation of that class (the unique direct subclass of the abc) is not installed.

[mkoeppe]

comment:9

Nils' question in https://groups.google.com/g/sage-devel/c/T0A4JCOg9DY/m/yybWDYd6BAAJ whether this new structure incurs a performance penalty can be tested for example on the method Polynomial.roots, which is changed in this ticket

[tscrim]

comment:11

Replying to @mkoeppe:

Replying to @tscrim:

I feel like this might go back towards the Ring subclass of Parent that we had before and have been trying to get away from by using the categories. Is there a specific reason why to put this back?

In #32414 I now explain that this is expressly not done in order to create a new hierarchy according to mathematical properties.

It's really just to make sure that code can test whether an object belongs to a class even if the implementation of that class (the unique direct subclass of the abc) is not installed.

So this is playing the role of a header file does in C? It is just a placeholder effectively saying "we will define this somewhere"?

What classes will we have to do this to? All of them? If so, I feels a bit clunky to maintain this manually. Could we do something so that this is automated? Should we try a different mechanism, such as a variation of lazy_import to replace the from foo import Bar for an isinstance(X, Bar) checks (equivalently an is_Bar function)?

[mkoeppe]

comment:12

Replying to @tscrim:

Replying to @mkoeppe:

Replying to @tscrim:

I feel like this might go back towards the Ring subclass of Parent that we had before and have been trying to get away from by using the categories. Is there a specific reason why to put this back?

In #32414 I now explain that this is expressly not done in order to create a new hierarchy according to mathematical properties.

It's really just to make sure that code can test whether an object belongs to a class even if the implementation of that class (the unique direct subclass of the abc) is not installed.

So this is playing the role of a header file does in C? It is just a placeholder effectively saying "we will define this somewhere"?

It's a way to think about it, yes. But it's a placeholder for one specific purpose: isinstance (or issubclass testing) from code in a module "far away" from the implementation of the class.

Here's a typical example. sage.groups.affine_groups.group_element.__call__ implements polymorphic behavior and imports is_Polyhedron because it wants to implement a specific way how to act on a polyhedron. Sometimes code of this kind can, of course, be seen as a workaround for API defects, or a need for further abstraction.

Anyway, in the proposed approach, we would change is_Polyhedron(v) to an import from sage.geometry.abc and use isinstance(v, sage.geometry.abc.Polyhedron).

So the proposed solution gives a way of enabling modularization (and reducing load times!) with minimal changes to the call site.

What classes will we have to do this to? All of them?

Absolutely not. Just those that appear in this kind of code.

Should we try a different mechanism, such as a variation of lazy_import to replace the from foo import Bar for an isinstance(X, Bar) checks (equivalently an is_Bar function)?

Reintroducing is_Bar functions runs counter to an effort eliminating them that has been ongoing since the early days of Sage (see #32414). Besides, also such is_Bar functions would need to be in a new, separate module that does not fail when it cannot import in the implementation module -- so try...except.

A variation of lazy_import is certainly something that could be explored as an alternative. We did something like this in #32455 for isinstance(..., cypari2.gen.Gen). But this is starting to look like piling on more workarounds like the ones that some modules do to break import cycles.

[kliem]

Reviewer: Jonathan Kliem

[kliem]

comment:13

LGTM.

[mkoeppe]

comment:14

Thanks!

[tscrim]

comment:15

Replying to @mkoeppe:

Replying to @tscrim:

So this is playing the role of a header file does in C? It is just a placeholder effectively saying "we will define this somewhere"?

It's a way to think about it, yes. But it's a placeholder for one specific purpose: isinstance (or issubclass testing) from code in a module "far away" from the implementation of the class.

Here's a typical example. sage.groups.affine_groups.group_element.__call__ implements polymorphic behavior and imports is_Polyhedron because it wants to implement a specific way how to act on a polyhedron. Sometimes code of this kind can, of course, be seen as a workaround for API defects, or a need for further abstraction.

Anyway, in the proposed approach, we would change is_Polyhedron(v) to an import from sage.geometry.abc and use isinstance(v, sage.geometry.abc.Polyhedron).

So the proposed solution gives a way of enabling modularization (and reducing load times!) with minimal changes to the call site.

Okay, thank you for the explanation. I am still slightly worried about scalability and maintenance cost, but we likely have to put it into practice in order to see if it doesn't work.

What classes will we have to do this to? All of them?

Absolutely not. Just those that appear in this kind of code.

I feel like this kind of pattern (calling something from "far away") is somewhat common, although I guess there aren't too many things that appear in other subfolders that it applies to. So perhaps it will not be so necessary. Although it might confuse some developers when they have to do this extra step if, say, they need a new algebra to compute something in the combinatorics folder.

Should we try a different mechanism, such as a variation of lazy_import to replace the from foo import Bar for an isinstance(X, Bar) checks (equivalently an is_Bar function)?

Reintroducing is_Bar functions runs counter to an effort eliminating them that has been ongoing since the early days of Sage (see #32414). Besides, also such is_Bar functions would need to be in a new, separate module that does not fail when it cannot import in the implementation module -- so try...except.

I am not suggesting we reintroduce them as I think we should just have more fundamental checks (when possible).

A variation of lazy_import is certainly something that could be explored as an alternative. We did something like this in #32455 for isinstance(..., cypari2.gen.Gen). But this is starting to look like piling on more workarounds like the ones that some modules do to break import cycles.

I feel like we are fighting the language more so than anything else. Anyways, it is good to try stuff and see what works. Thank you as always for your work on trying to improve Sage (even more at this scale).

[kliem]

comment:16

We are surely fighting that cython does not have preprocessor instructions. Would be really nice to have something like

try:
    from sage.rings.complex_double cimport ComplexDoubleField_class
except:
    ComplexDoubleField_class = None

Edit: Actually the above won't ever work, because it boils down to an optional build-dependency. The problem is that modularization will have the consequence that the above decision cannot be made at build-time.

[vbraun]

Changed branch from u/mkoeppe/sage_rings_abc to 6e2c3c4