Add ABCs CommutativePolynomial, MPolynomial_libsingular, InfinitePolynomial; deprecate is_Polynomial, is_MPolynomial
Closed this issue · 103 comments
isinstance(x, [M]Polynomial) replaces the use of is_Polynomial, is_MPolynomial (deprecated here).
The new class CommutativePolynomial is an ABC that is a common base of Polynomial, MPolynomial, and InfinitePolynomial. We introduce it here although there is currently no use for isinstance(x, CommutativePolynomial) in the library.
The new class InfinitePolynomial is a common base class of InfinitePolynomial_sparse and InfinitePolynomial_dense. Methods shared between both classes are moved to the new class.
The new class MPolynomial_libsingular is an ABCDEFIT (ABC defined exclusively for isinstance tests) with a unique direct subclass (see Meta-ticket #32414 and https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies for this design pattern). Its purpose is to replace uses of isinstance(x, MPolynomial_libsingular) throughout the library. This eliminates hard run-time dependencies on Singular.
We also make sage.rings.polynomial a namespace package; this is needed because element implementations depend on various libraries.
CC: @dimpase @mezzarobba @videlec @tscrim
Component: refactoring
Author: Matthias Koeppe
Reviewer: Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/32709
Author: Matthias Koeppe
Branch pushed to git repo; I updated commit sha1. New commits:
4185472 | Fixups |
Commit: 4185472
Description changed:
---
+++
@@ -1,4 +1,4 @@
-... replacing the use of `is_Polynomial`, `is_MPolynomial`.
+... replacing the use of `is_Polynomial`, `is_MPolynomial` (deprecated here).
Part of Meta-ticket #32414
Branch pushed to git repo; I updated commit sha1. New commits:
46d8351 | sage.rings.polynomial: Make it a namespace package |
Description changed:
---
+++
@@ -1,4 +1,6 @@
... replacing the use of `is_Polynomial`, `is_MPolynomial` (deprecated here).
+
+We also make `sage.rings.polynomial` a namespace package; this is needed because element implementations depend on various libraries.
Part of Meta-ticket #32414
Merge failure.
Branch pushed to git repo; I updated commit sha1. New commits:
e156cad | Merge tag '9.8.beta6' into t/32709/sage_structure_element__add_abcs_polynomial__mpolynomial_for_isinstance_testing |
Thank you for doing this. This is something that I have been thinking about having for a while. The only other thing I would like would be a common ABC for both univariate and multivariate polynomials.
Currently we have:
cdef class Polynomial(CommutativeAlgebraElement):
but
cdef class MPolynomial(CommutativeRingElement):
The branch here is changing it (I think this is harmless). So we could just introduce a common base class -- but what would that be called?
That is a bit strange to me. They both are algebras. From the class hierarchy, inheriting the subclass should make it a bit better.
I might call it Polynomial_base.
How about AnyPolynomial?
(In this ticket, I'm already using Polynomial_base and MPolynomial_base)
Or maybe CommutativePolynomial?
CommutativePolynomial is good. AnyPolynomial feels somewhat awkward for an ABC and a bit too generic.
Ideally I would like to rename Polynomial to UPolynomial or something saying it is univariate. Just for the base class. Although that upends a fair amount of the naming conventions currently in Sage...
Would InfinitePolynomial_sparse and InfinitePolynomial_dense also become subclasses of CommutativePolynomial? (Currently they directly subclass RingElement)
Replying to Travis Scrimshaw:
Ideally I would like to rename
PolynomialtoUPolynomialor something saying it is univariate. Just for the base class. Although that upends a fair amount of the naming conventions currently in Sage...
I think that's too intrusive a change; at least for this ticket
Replying to Matthias Köppe:
Would
InfinitePolynomial_sparseandInfinitePolynomial_densealso become subclasses ofCommutativePolynomial? (Currently they directly subclassRingElement)
That's a good point. Yes, I think they should. They should act in the same way as other polynomials.
Replying to Matthias Köppe:
Replying to Travis Scrimshaw:
Ideally I would like to rename
PolynomialtoUPolynomialor something saying it is univariate. Just for the base class. Although that upends a fair amount of the naming conventions currently in Sage...I think that's too intrusive a change; at least for this ticket
Indeed.
OK. I'll push these changes to the ticket in a bit.
Thank you!
I don't think we want a special ABC for the "infinite" polynomial as the name suggests they are formal power series, and the ring is not different at an abstract level (just in implementation) from a usual multivariate polynomial.
We probably should also update the hierarchy documentation at the top of element.pyx.
The ABCs in sage.structure.element are introduced for isinstance testing (#32414). Though currently there is no reference to the InfinitePolynomial_* classes outside of sage.rings.polynomial.infinite_polynomial*, the new ABC makes such tests possible
I do agree that the naming InfinitePolynomial can be misleading.
Branch pushed to git repo; I updated commit sha1. New commits:
4050551 | src/sage/structure/element.pyx: Update hierarchy in documentation |
Replying to Matthias Köppe:
The ABCs in
sage.structure.elementare introduced forisinstancetesting (#32414). Though currently there is no reference to theInfinitePolynomial_*classes outside ofsage.rings.polynomial.infinite_polynomial*, the new ABC makes such tests possible
But you can just do the isinstance testing against the InfinitePolynomial_sparse from that file. They are just a specific implementation of a multivariate polynomial ring in my mind.
This is for modularization purposes: avoiding to have to import an implementation module just to test whether an object may be coming from that module. (see https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies)
By that logic, we should have every class with a corresponding abstract class in element.pyx. Is that what you're advocating for here?
Every class would be too much and is not needed. But some implementation classes, in particular polynomials, vectors, matrices benefit from this as they are used in lots of places
To me, if someone wants to specifically test against the InfinitePolynomialRing elements, they are almost certainly going to be doing something involving that class and need to import it. This isn't a generic class. Would you do the same for a polynomial ring with generators indexed by different uncountable sets?
The importing can often be done when isinstance has returned True
Typical for example in element constructors where you want to handle conversions from a long list of element classes defined elsewhere. Instead of having to import all of these modules providing the implementation classes (which may be pulling in dependencies), you can do the isinstance testing against the ABC.
Per meta-ticket #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". The new ABCs Polynomial, MPolynomial, InfinitePolynomial fit this pattern (the new ABC CommutativePolynomial obviously does not)
It seems like you're back to saying everything should have an ABC. What's the cutoff that you are using to distinguish between things that should have one and those that don't? Should all of the specific polynomial implementations have corresponding ABCs? This seems like a huge maintenance burden to duplicate these classes that can easily get out of sync.
I'm introducing them as needed for the modularization.
git grep 'isinstance.*Polynomial_.*_' shows only a small number of isinstance tests with specific implementation classes. Some of them will indeed need similar work, but that is independent of the deprecation of is_... functions.
I don't see how this extra class for infinitely generated polynomial rings helps that. It seems to be for a future problem that might not ever occur. As you said, for this class, it is never called outside of the files containing the parent and element definitions. (The key polynomials ticket will be explicitly be using them.) As such, adding this extra ABC is independent of the deprecation.
If you really are planning to have many classes with specific intermediate ABCs that need to be in 1-1 correspondence with concrete implementations, then they should have specific names and documentation to indicate that they should not be inherited from other classes.
For (multivariate) polynomials, it is natural to have these ABCs, where the implementor does not want to use what Sage's current ABCs already provide (as they provide partial implementations). I have this in some personal code that I haven't get prepped for inclusion into Sage.
If you want to consider the infinitely-generated polynomial ring elements as not being a specific implementation of the MPolynomial ABC, but instead deserve their own ABC, then can you give some justification for this?
Replying to Travis Scrimshaw:
I don't see how this extra class for infinitely generated polynomial rings helps that. It seems to be for a future problem that might not ever occur.
That's right. I only introduced it because having it right next to the other classes in sage.structure.element helps clarify the nature of the CommutativePolynomial ABC
Replying to Travis Scrimshaw:
If you really are planning to have many classes with specific intermediate ABCs that need to be in 1-1 correspondence with concrete implementations, then they should have specific names and documentation to indicate that they should not be inherited from other classes.
I have such doctests for the ABCs in sage.rings.abc. I'll add such doctests here too.
Replying to Travis Scrimshaw:
If you want to consider the infinitely-generated polynomial ring elements as not being a specific implementation of the
MPolynomialABC, but instead deserve their own ABC, then can you give some justification for this?
I don't know what ABC the InfinitePolynomial_* classes implement. There's nothing that makes the promise that it is compatible with MPolynomial.
Replying to Matthias Köppe:
Replying to Travis Scrimshaw:
If you want to consider the infinitely-generated polynomial ring elements as not being a specific implementation of the
MPolynomialABC, but instead deserve their own ABC, then can you give some justification for this?I don't know what ABC the
InfinitePolynomial_*classes implement. There's nothing that makes the promise that it is compatible withMPolynomial.
They aren't ABCs, they are actual concrete implementations.
sage: R.<x> = InfinitePolynomialRing(QQ)
sage: type(x[1])
<class 'sage.rings.polynomial.infinite_polynomial_element.InfinitePolynomial_dense'>
sage: R.<x> = InfinitePolynomialRing(QQ, implementation='sparse')
sage: type(x[1])
<class 'sage.rings.polynomial.infinite_polynomial_element.InfinitePolynomial_sparse'>
They are just wrappers around normal multivariate polynomials that grow the number of variables as needed. They are supposed to behave the same as other multivariate polynomials because of this. Plus we have flexibility with the added MPolynomial ABC, which we do not have to impose that it is finitely generated.
Branch pushed to git repo; I updated commit sha1. New commits:
52da2d1 | src/sage/structure/element.pyx: Add unique-direct-subclass tests |
Replying to Travis Scrimshaw:
They are just wrappers around normal multivariate polynomials that grow the number of variables as needed.
Yes, but I don't think they support all methods that multivariate polynomials support.
Replying to Travis Scrimshaw:
Plus we have flexibility with the added
MPolynomialABC, which we do not have to impose that it is finitely generated.
No, that is not what the MPolynomial ABC is doing. It's documented now that it has a unique direct subclass
Replying to Matthias Köppe:
Replying to Travis Scrimshaw:
They are just wrappers around normal multivariate polynomials that grow the number of variables as needed.
Yes, but I don't think they support all methods that multivariate polynomials support.
They should unless it is something that depends on being finitely generated. I am not sure how much depends on that offhand.
Oh, I see, it's forwarding method calls using custom __getattr__.
And indeed the wrapped polynomial always belongs to an MPolynomialRing (even if there's only 1 variable.)
Replying to Matthias Köppe:
Replying to Travis Scrimshaw:
Plus we have flexibility with the added
MPolynomialABC, which we do not have to impose that it is finitely generated.No, that is not what the
MPolynomialABC is doing. It's documented now that it has a unique direct subclass
That was not clear before. I think we should have a proper ABC, in the actual sense of the term, for (multivariate) polynomials.
That also brings about another maintenance burden: we are manually having to update the class hierarchy documentation. Although because they are 1-1 with more concrete implementations, they shouldn't appear there.
We need another term other than ABC for such classes. Maybe adding an adjective.
We also need to have criteria/rules for when we need to add such a class and when not.
How do you plan to deal with multiple inheritance with Cython:
A < B
| |
a < b
where A,B are the 1-1ABCs and a and b are the (more) concrete classes.
Replying to Travis Scrimshaw:
We need another term other than ABC for such classes. Maybe adding an adjective.
I agree, that would be an improvement. Perhaps an ABCDEFIT, "ABC defined exclusively for isinstance test"
Replying to Travis Scrimshaw:
How do you plan to deal with multiple inheritance with Cython:
A < B | | a < bwhere
A,Bare the 1-1ABCs andaandbare the (more) concrete classes.
No problem there; the ABCDEFITs can also just be Python mixin classes.
Replying to Travis Scrimshaw:
I think we should have a proper ABC, in the actual sense of the term, for (multivariate) polynomials.
In fact we should probably have a proper ABC that unifies univariate and multivariate polynomials. Definitely not this ticket.
Replying to Travis Scrimshaw:
That also brings about another maintenance burden: we are manually having to update the class hierarchy documentation. Although because they are 1-1 with more concrete implementations, they shouldn't appear there.
How about I mark the ABCDEFITs in the documentation of this module (the new *Polynomial and the existing Expression) with some symbol and link to the developer's guide for the explanation of this design?
Replying to Matthias Köppe:
Replying to Travis Scrimshaw:
How do you plan to deal with multiple inheritance with Cython:
A < B | | a < bwhere
A,Bare the 1-1ABCs andaandbare the (more) concrete classes.No problem there; the ABCDEFITs can also just be Python mixin classes.
There is a bit of a performance one. You have to add another Python class corresponding to the actual classes you instantiate. So that Cython code has to go through those Python objects, not Cython, which adds a performance penalty as I understand it. (Plus there is yet another essentially empty class.)
Replying to Matthias Köppe:
Replying to Travis Scrimshaw:
I think we should have a proper ABC, in the actual sense of the term, for (multivariate) polynomials.
In fact we should probably have a proper ABC that unifies univariate and multivariate polynomials. Definitely not this ticket.
That was what the CommutativePolynomial is going towards.
Replying to Matthias Köppe:
Replying to Travis Scrimshaw:
That also brings about another maintenance burden: we are manually having to update the class hierarchy documentation. Although because they are 1-1 with more concrete implementations, they shouldn't appear there.
How about I mark the ABCDEFITs in the documentation of this module (the new
*Polynomialand the existingExpression) with some symbol and link to the developer's guide for the explanation of this design?
I would find that acceptable provided that we have a discussion on sage-devel about this since this implies a major structural change.
Branch pushed to git repo; I updated commit sha1. New commits:
c72dcc8 | src/sage/structure/element.pyx: Add unique-direct-subclass test for Expression |
Replying to Travis Scrimshaw:
There is a bit of a performance one. You have to add another Python class corresponding to the actual classes you instantiate. So that Cython code has to go through those Python objects, not Cython, which adds a performance penalty as I understand it.
I'm not concerned about this
Similar changes in #32664 may be easier to review, as it is just using an existing ABC to get rid of the is_... functions
Replying to Travis Scrimshaw:
Replying to Matthias Köppe:
How about I mark the ABCDEFITs in the documentation of this module (the new
*Polynomialand the existingExpression) with some symbol and link to the developer's guide for the explanation of this design?I would find that acceptable provided that we have a discussion on sage-devel about this since this implies a major structural change.
What's the new quality of change compared to the previous #32638, #32606, etc.?
Replying to Matthias Köppe:
Replying to Travis Scrimshaw:
Replying to Matthias Köppe:
How about I mark the ABCDEFITs in the documentation of this module (the new
*Polynomialand the existingExpression) with some symbol and link to the developer's guide for the explanation of this design?I would find that acceptable provided that we have a discussion on sage-devel about this since this implies a major structural change.
What's the new quality of change compared to the previous #32638, #32606, etc.?
I missed it for expression as I thought it was meant for more general implementations rather than a specific one. For the integer mod ring, we have different implementations, so I was thinking it was again a common ABC, not an ABEDEFIT. Mainly, I thought I was allowed to extend them and could see a reason for doing it. Here, we are doing it for something that IMO is for one specific implementation that at this point will likely not be used in a more abstract/generic way.
That I am creating ABCDEFITs here and in the tickets of #32414 is basically to limit the scope of the tickets. There is nothing that would stop us from upgrading an ABCDEFIT to a "proper" ABC at some point in the future when there is a need for that. In this case we would remove the uniqueness doctest.
Description changed:
---
+++
@@ -1,6 +1,8 @@
-... replacing the use of `is_Polynomial`, `is_MPolynomial` (deprecated here).
+The new classes `Polynomial` and `MPolynomial` are ABCs with a unique direct subclass (see Meta-ticket #32414 and https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies for this design pattern). Their purpose is that `isinstance(x, sage.structure.element.[M]Polynomial)` replaces the use of `is_Polynomial`, `is_MPolynomial` (deprecated here).
+
+The new class `InfinitePolynomial` is also an ABC with a unique direct subclass. We introduce it here although there is currently no use of `isinstance(x, InfinitePolynomial...)` in the library.
+
+The new class `CommutativePolynomial` is an ABC that is a common base of `Polynomial`, `MPolynomial`, and `InfinitePolynomial`. We introduce it here although there is currently no use for `isinstance(x, CommutativePolynomial)` in the library.
We also make `sage.rings.polynomial` a namespace package; this is needed because element implementations depend on various libraries.
-Part of Meta-ticket #32414
-But to do that, don't we need to add an additional proper ABC on top of the ABCDEFIT?
I still do not see why the InfinitePolynomialRing deserves special treatment. Also why does it not inherit from a proper MPolynomial ABC?
Replying to Travis Scrimshaw:
I still do not see why the
InfinitePolynomialRingdeserves special treatment.
I don't think it's treated more specially than the other two subclasses in this branch?
Also why does it not inherit from a proper
MPolynomialABC?
I don't know why the implementation class InfinitePolynomial_sparse is not a subclass of the implementation class MPolynomial, but it isn't.
Replying to Matthias Köppe:
Replying to Travis Scrimshaw:
I still do not see why the
InfinitePolynomialRingdeserves special treatment.I don't think it's treated more specially than the other two subclasses in this branch?
I guess that is the point I am a bit confused about. Why can't the MPolynomial in element.pyx be also used as a base class for InfinitePolynomial_sparse.
But let's assume that MPolynomial is an ABCDEFIT. The big difference is that the actual concrete implementations of Polynomial and MPolynomial are still ABCs, whereas InfinitePolynomial_sparse is a specific concrete implementation. Why not do the same thing for, say, the libsingular polynomials (in this case, this is explicitly tested against in the algebras/fusion_ring code)?
Also why does it not inherit from a proper
MPolynomialABC?I don't know why the implementation class
InfinitePolynomial_sparseis not a subclass of the implementation classMPolynomial, but it isn't.
My guess is that there are a few methods that assume that the ring is finitely generated (e.g., gradient()) and nobody thought to bring these together beforehand. Some slight refactoring would be needed, but it shouldn't be too hard.
Replying to Travis Scrimshaw:
But let's assume that
MPolynomialis an ABCDEFIT. The big difference is that the actual concrete implementations ofPolynomialandMPolynomialare still ABCs, whereasInfinitePolynomial_sparseis a specific concrete implementation.
Note that InfinitePolynomial_dense is a subclass of InfinitePolynomial_sparse. It would perhaps be cleaner if both were subclasses of an ABC InfinitePolynomial_base.
Why not do the same thing for, say, the libsingular polynomials (in this case, this is explicitly tested against in the
algebras/fusion_ringcode)?
Right, I do see the isinstance(fvar, MPolynomial_libsingular) in src/sage/algebras/fusion_rings/shm_managers.pyx, and this isinstance test also shows up in a bunch of random modules elsewhere.
From a quick look it is likely that indeed it would also be good to introduce an ABCDEFIT for it to get rid of this obstacle to modularization.
I'll be happy to take care of this here on the ticket, although it widens the original scope of the ticket even more (as there is no corresponding is_... function).
If you don't mind doing it, then please go ahead. It would definitely be nice to factor out the InfinitePolynomial_* into a common ABC. I think it helps make clear about when to include ABCDEFITs.
I am happy to do some extra reviewing. It might be more of a hassle to split off the new parts into a separate ticket.
Branch pushed to git repo; I updated commit sha1. New commits:
9dba2c7 | InfinitePolynomial: Change from constructor function to base class with __classcall__ |
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
6696f5c | Use ABC MPolynomial for isinstance testing |
79f7e62 | Fixups |
ed050bb | sage.rings.polynomial: Make it a namespace package |
0613280 | sage.structure.element: Introduce common base class CommutativePolynomial for ABCs Polynomial, MPolynomial |
27e8889 | sage.structure.element: Introduce ABC InfinitePolynomial |
57228f5 | src/sage/structure/element.pyx: Update hierarchy in documentation |
95e0adc | src/sage/structure/element.pyx: Add unique-direct-subclass tests |
1fd625c | src/sage/structure/element.pyx: Add unique-direct-subclass test for Expression |
960506b | InfinitePolynomial: Change from constructor function to base class with __classcall__ |
8c6bf18 | WIP - Change design |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
19255e8 | WIP - Change design |
I'm moving the modularization boundary into sage.rings.polynomial. The additions to sage.structure.element are gone.
The new module sage.rings.polynomial.commutative_polynomial provides a new, currently trivial, ABC.
Description changed:
---
+++
@@ -1,8 +1,8 @@
-The new classes `Polynomial` and `MPolynomial` are ABCs with a unique direct subclass (see Meta-ticket #32414 and https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies for this design pattern). Their purpose is that `isinstance(x, sage.structure.element.[M]Polynomial)` replaces the use of `is_Polynomial`, `is_MPolynomial` (deprecated here).
-
-The new class `InfinitePolynomial` is also an ABC with a unique direct subclass. We introduce it here although there is currently no use of `isinstance(x, InfinitePolynomial...)` in the library.
+`isinstance(x, [M]Polynomial)` replaces the use of `is_Polynomial`, `is_MPolynomial` (deprecated here).
The new class `CommutativePolynomial` is an ABC that is a common base of `Polynomial`, `MPolynomial`, and `InfinitePolynomial`. We introduce it here although there is currently no use for `isinstance(x, CommutativePolynomial)` in the library.
+The new class `MPolynomial_libsingular` is an ABCDEFIT (ABC defined exclusively for `isinstance` tests) with a unique direct subclass (see Meta-ticket #32414 and https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies for this design pattern). Its purpose is to replace uses of `isinstance(x, MPolynomial_libsingular)` throughout the library. This eliminates hard run-time dependencies on Singular.
+
We also make `sage.rings.polynomial` a namespace package; this is needed because element implementations depend on various libraries.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0b6d19e | Remove ABCs sage.structure.element.*Polynomial; introduce ABCs in sage.rings.polynomial |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fd54c82 | Remove ABCs sage.structure.element.*Polynomial; introduce ABCs in sage.rings.polynomial |
Description changed:
---
+++
@@ -1,6 +1,8 @@
`isinstance(x, [M]Polynomial)` replaces the use of `is_Polynomial`, `is_MPolynomial` (deprecated here).
The new class `CommutativePolynomial` is an ABC that is a common base of `Polynomial`, `MPolynomial`, and `InfinitePolynomial`. We introduce it here although there is currently no use for `isinstance(x, CommutativePolynomial)` in the library.
+
+The new class `InfinitePolynomial` is a common base class of `InfinitePolynomial_sparse` and `InfinitePolynomial_dense`. Methods shared between both classes are moved to the new class.
The new class `MPolynomial_libsingular` is an ABCDEFIT (ABC defined exclusively for `isinstance` tests) with a unique direct subclass (see Meta-ticket #32414 and https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies for this design pattern). Its purpose is to replace uses of `isinstance(x, MPolynomial_libsingular)` throughout the library. This eliminates hard run-time dependencies on Singular.
Thank you. Last two things, both for the infinite polynomials:
__classcall_private__needs doctests.- The
_rmul_and_lmul_can be refactored out to usetype(self).
Thank you. LGTM.
Reviewer: Travis Scrimshaw