cleaning CategoryObject/Parent - Githubissues

sagemath / sage

Main repository of SageMath

https://www.sagemath.org

Other

1.32k stars 453 forks source link

cleaning CategoryObject/Parent #21380

Open videlec opened 8 years ago

videlec commented 8 years ago

Abstract

This ticket stands to clean CategoryObject and Parent with respect to:

hacks and too specialized methods (e.g. base or _ngens_)
undocumented and old (e.g. sage.structure.generators)
duplicated stuff (e.g. _an_element_ and an_element)

Subtasks

Base and generators

We should move out of CategoryObject the attributes and methods related to generators and variable names.

Attributes:

_names

Methods:

gens_dict
gens_dict_recursive
objgens
objgen
_first_ngens
_defining_names
_assign_names
__temporarily_change_names
variable_names
variable_name
latex_variable_names
latex_name
inject_variables
base_ring

It is not clear where all this should be moved... Moreover "generators" is ambiguous since "monoid generators", "group generators", "algebra generators", ... are different things.

Keep

These were originally proposed to be removed too, but are sufficiently useful:

Attributes:

_base Methods:
base Functions:
normalize_names
certify_names

Tickets

Some related tickets are

gradual transition for to remove inheritance from ParentWithGens to Ring (#13683? still useful?)
24430: normalization for _names, variable_name and variable_names in the context of polynomials and series

Transition to `FiniteEnumeratedSet`

set the category to FiniteEnumeratedSets where appropriate (#12957)
Move __len__ and __getitem__ from Parent to FiniteEnumeratedSets (#12955)
deprecate CombinatorialClass in favor of the EnumeratedSets category (#12913, #19986)

Duplicated names for same function

cleanup an_element (#18291)
cardinality vs order (#18410)

Others

move Set_generic and Set_Python_type_class out of sage.structure.parent. Moreover we should use UniqueRepresentation instead of the custom class factory Set_PythonType.
Various fixes in category initialization (#21353)
cleanup variable factories (#18390)
cleanup cartesian products (task ticket #15425)

Closed tickets

refactor getattr_from_other_class (#20686)
Simplify _populate_generators_ (#21381)
Remove sage.structure.generators (#21382)
Remove ParentWith*AbelianGens (#21383)
Remove _populate_generators_ (#21385)
Always enable debug.bad_parent_warnings (#24109)

CC: @tscrim

Component: categories

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

jdemeyer commented 8 years ago

comment:1

Are you sure this would affect Element much? All the things you write are about CategoryObject or Parent.

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,6 @@
 ## Abstract

-This ticket stands to clean `CategoryObject`, `Parent` and `Element` with respect to:
+This ticket stands to clean `CategoryObject` and `Parent` with respect to:
 - hacks and too specialized methods (e.g. `base` or `_ngens_`)
 - undocumented and old (e.g. `sage.structure.generators`)
 - duplicated stuff (e.g. `_an_element_` and `an_element`)
@@ -9,7 +9,7 @@

 ### Base and generators

-We should move aout of `CategoryObject` the attributes and method relative to "base" and "generators". There are 3 such attributes
+We should move out of `CategoryObject` the attributes and methods relative to "base" and "generators". There are 3 such attributes
 - `_generators`
 - `_base`
 - `_names`
@@ -52,7 +52,7 @@

 ### Duplicated names for same function

-- cleanup an_element (#18291)
+- cleanup `an_element` (#18291)

 - cardinality vs order (#18410)

@@ -60,7 +60,7 @@

 - move `Set_generic` and `Set_Python_type_class` out of `sage.structure.parent`. Moreover we should use `UniqueRepresentation` instead of the custom class factory `Set_PythonType`.

-- refactor getattr_from_other_class (#20686)
+- refactor `getattr_from_other_class` (#20686)

 - get rid of `guess_category` (#21353)

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -13,7 +13,7 @@
 - `_generators`
 - `_base`
 - `_names`
-  And 21 such methods
+  And 21 methods
 - `_populate_generators_`
 - `_ngens_` (used nowhere out of `category_object.pyx`!!)
 - `gens_dict`
@@ -34,6 +34,11 @@
 - `has_base`
 - `base_ring`
 - `base`
+  And 2 functions
+- `normalize_names`
+- `certify_names`
+  And 1 class
+- `localvars`

 It is not clear where all this should be moved... Moreover "generators" is ambiguous since "monoid generators", "group generators", "algebra generators", ... are different things.

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -15,7 +15,7 @@
 - `_names`
   And 21 methods
 - `_populate_generators_`
-- `_ngens_` (used nowhere out of `category_object.pyx`!!)
+- `_ngens_` (always returns zero!!)
 - `gens_dict`
 - `gens_dict_recursive`
 - `objgens`

videlec commented 8 years ago

Description changed:

--- 
+++ 
@@ -72,3 +72,5 @@
 - cleanup variable factories  (#18390)

 - cleanup cartesian products (task ticket #15425)
+
+- simplify `_populate_generators_` (#21381)

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -74,3 +74,5 @@
 - cleanup cartesian products (task ticket #15425)

 - simplify `_populate_generators_` (#21381)
+
+- remove unused classes from `generators.pyx` (#21382)

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -75,4 +75,6 @@

 - simplify `_populate_generators_` (#21381)

-- remove unused classes from `generators.pyx` (#21382)
+- Remove unused classes from `generators.pyx` (#21382)
+
+- Remove ParentWith*AbelianGens (#21383)

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -73,8 +73,8 @@

 - cleanup cartesian products (task ticket #15425)

-- simplify `_populate_generators_` (#21381)
+- Simplify `_populate_generators_` (#21381)

 - Remove unused classes from `generators.pyx` (#21382)

-- Remove ParentWith*AbelianGens (#21383)
+- Remove `ParentWith*AbelianGens` (#21383)

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -75,6 +75,6 @@

 - Simplify `_populate_generators_` (#21381)

-- Remove unused classes from `generators.pyx` (#21382)
+- Remove `sage.structure.generators` (#21382)

 - Remove `ParentWith*AbelianGens` (#21383)

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -43,7 +43,6 @@
 It is not clear where all this should be moved... Moreover "generators" is ambiguous since "monoid generators", "group generators", "algebra generators", ... are different things.

 Some Related tickets are
-- improve coverage of `generators.pyx` (#5768? still useful?)

 - gradual transition for to remove inheritance from `ParentWithGens` to `Ring` (#13683? still useful?)

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -10,12 +10,12 @@
 ### Base and generators

 We should move out of `CategoryObject` the attributes and methods relative to "base" and "generators". There are 3 such attributes
-- `_generators`
+- `_generators`: removed in #21382
 - `_base`
 - `_names`
   And 21 methods
 - `_populate_generators_`
-- `_ngens_` (always returns zero!!)
+- `_ngens_`: removed in #21381
 - `gens_dict`
 - `gens_dict_recursive`
 - `objgens`

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -14,7 +14,7 @@
 - `_base`
 - `_names`
   And 21 methods
-- `_populate_generators_`
+- `_populate_generators_`: removed in #21385
 - `_ngens_`: removed in #21381
 - `gens_dict`
 - `gens_dict_recursive`
@@ -77,3 +77,5 @@
 - Remove `sage.structure.generators` (#21382)

 - Remove `ParentWith*AbelianGens` (#21383)
+
+- Remove `_populate_generators_` (#21385)

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -30,7 +30,7 @@
 - `latex_variable_names`
 - `latex_name`
 - `inject_variables`
-- `injvar`
+- `injvar`: deprecated (#4143)
 - `has_base`
 - `base_ring`
 - `base`

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -30,7 +30,7 @@
 - `latex_variable_names`
 - `latex_name`
 - `inject_variables`
-- `injvar`: deprecated (#4143)
+- `injvar`: deprecated since 8 years! (#4143)
 - `has_base`
 - `base_ring`
 - `base`

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -24,7 +24,7 @@
 - `_defining_names`
 - `_assign_names`
 - `__temporarily_change_names`
-- `_temporarily_change_names`
+- `_temporarily_change_names`: unused
 - `variable_names`
 - `variable_name`
 - `latex_variable_names`
@@ -38,7 +38,7 @@
 - `normalize_names`
 - `certify_names`
   And 1 class
-- `localvars`
+- `localvars`: unused

 It is not clear where all this should be moved... Moreover "generators" is ambiguous since "monoid generators", "group generators", "algebra generators", ... are different things.

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -10,12 +10,12 @@
 ### Base and generators

 We should move out of `CategoryObject` the attributes and methods relative to "base" and "generators". There are 3 such attributes
-- `_generators`: removed in #21382
+- `_generators`: unused, removed in #21382
 - `_base`
 - `_names`
   And 21 methods
-- `_populate_generators_`: removed in #21385
-- `_ngens_`: removed in #21381
+- `_populate_generators_`: unused, removed in #21385
+- `_ngens_`: unused, removed in #21381
 - `gens_dict`
 - `gens_dict_recursive`
 - `objgens`
@@ -24,13 +24,13 @@
 - `_defining_names`
 - `_assign_names`
 - `__temporarily_change_names`
-- `_temporarily_change_names`: unused
+- `_temporarily_change_names`: unused, removed in #21395
 - `variable_names`
 - `variable_name`
 - `latex_variable_names`
 - `latex_name`
 - `inject_variables`
-- `injvar`: deprecated since 8 years! (#4143)
+- `injvar`: unused, removed in #21395
 - `has_base`
 - `base_ring`
 - `base`
@@ -38,7 +38,7 @@
 - `normalize_names`
 - `certify_names`
   And 1 class
-- `localvars`: unused
+- `localvars`: unused, removed in #21395

 It is not clear where all this should be moved... Moreover "generators" is ambiguous since "monoid generators", "group generators", "algebra generators", ... are different things.

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -31,7 +31,7 @@
 - `latex_name`
 - `inject_variables`
 - `injvar`: unused, removed in #21395
-- `has_base`
+- `has_base`: unused, deprecated in #21395
 - `base_ring`
 - `base`
   And 2 functions

jdemeyer commented 6 years ago

Replying to @videlec:

We should move out of CategoryObject the attributes and methods relative to "base"

After thinking about this now and then, I think that it does make sense to keep _base inside CategoryObject. There are a lot of objects which naturally have a base object. For efficiency and simplicity, I suggest to keep CategoryObject._base.

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -9,13 +9,13 @@

 ### Base and generators

-We should move out of `CategoryObject` the attributes and methods relative to "base" and "generators". There are 3 such attributes
-- `_generators`: unused, removed in #21382
+We should move out of `CategoryObject` the attributes and methods relative to "base" and "generators".
+
+Attributes:
 - `_base`
 - `_names`
-  And 21 methods
-- `_populate_generators_`: unused, removed in #21385
-- `_ngens_`: unused, removed in #21381
+
+Methods:
 - `gens_dict`
 - `gens_dict_recursive`
 - `objgens`
@@ -24,25 +24,20 @@
 - `_defining_names`
 - `_assign_names`
 - `__temporarily_change_names`
-- `_temporarily_change_names`: unused, removed in #21395
 - `variable_names`
 - `variable_name`
 - `latex_variable_names`
 - `latex_name`
 - `inject_variables`
-- `injvar`: unused, removed in #21395
-- `has_base`: unused, deprecated in #21395
 - `base_ring`
 - `base`
-  And 2 functions
+Functions:
 - `normalize_names`
 - `certify_names`
-  And 1 class
-- `localvars`: unused, removed in #21395

 It is not clear where all this should be moved... Moreover "generators" is ambiguous since "monoid generators", "group generators", "algebra generators", ... are different things.

-Some Related tickets are
+Some related tickets are

 - gradual transition for to remove inheritance from `ParentWithGens` to `Ring` (#13683? still useful?)

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -59,13 +59,15 @@

 - move `Set_generic` and `Set_Python_type_class` out of `sage.structure.parent`. Moreover we should use `UniqueRepresentation` instead of the custom class factory `Set_PythonType`.

-- refactor `getattr_from_other_class` (#20686)
-
-- get rid of `guess_category` (#21353)
+- Various fixes in category initialization (#21353)

 - cleanup variable factories  (#18390)

 - cleanup cartesian products (task ticket #15425)
+
+### Closed tickets
+
+- refactor `getattr_from_other_class` (#20686)

 - Simplify `_populate_generators_` (#21381)

@@ -74,3 +76,6 @@
 - Remove `ParentWith*AbelianGens` (#21383)

 - Remove `_populate_generators_` (#21385)
+
+- Always enable `debug.bad_parent_warnings` (#24109)
+

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -9,10 +9,9 @@

 ### Base and generators

-We should move out of `CategoryObject` the attributes and methods relative to "base" and "generators".
+We should move out of `CategoryObject` the attributes and methods related to generators and variable names.

 Attributes:
-- `_base`
 - `_names`

 Methods:
@@ -30,12 +29,22 @@
 - `latex_name`
 - `inject_variables`
 - `base_ring`
+
+It is not clear where all this should be moved... Moreover "generators" is ambiguous since "monoid generators", "group generators", "algebra generators", ... are different things.
+
+### Keep
+
+These were originally proposed to be removed too, but are sufficiently useful:
+
+Attributes:
+- `_base`
+Methods:
 - `base`
 Functions:
 - `normalize_names`
 - `certify_names`

-It is not clear where all this should be moved... Moreover "generators" is ambiguous since "monoid generators", "group generators", "algebra generators", ... are different things.
+### Tickets

 Some related tickets are

jdemeyer commented 6 years ago

comment:21

What is wrong with the functions normalize_names() and certify_names? They have to be put somewhere, why not in structure/category_object.pyx?

videlec commented 6 years ago

comment:22

Replying to @jdemeyer:

Replying to @videlec:

We should move out of CategoryObject the attributes and methods relative to "base"

After thinking about this now and then, I think that it does make sense to keep _base inside CategoryObject. There are a lot of objects which naturally have a base object. For efficiency and simplicity, I suggest to keep CategoryObject._base.

Some does, some does not and there is no specification of what a base is. I would be happy to hear a definition.

videlec commented 6 years ago

comment:23

Replying to @jdemeyer:

What is wrong with the functions normalize_names() and certify_names? They have to be put somewhere, why not in structure/category_object.pyx?

Right (I made the listing rather quickly)

jdemeyer commented 6 years ago

comment:24

Replying to @videlec:

Some does, some does not and there is no specification of what a base is. I would be happy to hear a definition.

Well, we cannot define "base" in full generality. Some objects do not have a natural base and then this attribute should simply be None.

But in certain important categories, we can define "base". For example, for modules, we define "base" to mean the parent of the scalars used for scalar multiplication.

videlec commented 6 years ago

comment:25

Replying to @jdemeyer:

Replying to @videlec:

Some does, some does not and there is no specification of what a base is. I would be happy to hear a definition.

Well, we cannot define "base" in full generality. Some objects do not have a natural base and then this attribute should simply be None.

But in certain important categories, we can define "base". For example, for modules, we define "base" to mean the parent of the scalars used for scalar multiplication.

This is precisely the reason why I dislike it. It is a fake global concept that is mostly meant for "the underlying ring of a module". And because modules are important all parents have an attribute _base as well as methods base and base_ring. Think for a minute about the list of parents without base: permutations, graphs, groups, rings, schemes, simplicial complexes, ...

Moreover, I (maybe wrongly) thought that this "base ring of a module" is the kind of information that belongs to the category

sage: Modules(ZZ)
Category of modules over Integer Ring
sage: Modules(ZZ).base()
Integer Ring

jdemeyer commented 6 years ago

comment:26

Replying to @videlec:

Think for a minute about the list of parents without base: permutations, graphs, groups, rings, schemes, simplicial complexes, ...

Some rings and schemes do have a concept of "base"...

videlec commented 6 years ago

comment:27

Replying to @jdemeyer:

Replying to @videlec:

Think for a minute about the list of parents without base: permutations, graphs, groups, rings, schemes, simplicial complexes, ...

Some rings and schemes do have a concept of "base"...

Some graphs also (Cayley graphs) as well as groups (GL(2,Z)) etc

jdemeyer commented 6 years ago

comment:28

Replying to @videlec:

It is a fake global concept that is mostly meant for "the underlying ring of a module".

Note that there are a lot of parents in Sage which represent a module of some sort. Most of them do not inherit from Module. So even if we implement _base in the Module class, that won't help unless we change a large number of parents to inherit from that.

tscrim commented 6 years ago

comment:29

Ideally base would be something that belongs to the appropriate categories. The reason I am not currently proposing this is that I'm very worried about the speed regression of making this a pure Python function call.

I am starting to wonder if we do want to actually have (new-style) Parent, ParentWithBase, ParentWithGens, and possibly ParentWithBaseGens classes that give these various features as common ABCs. While this would probably be an invasive change, it should not be too painful (and should be easy enough to deprecate). For those very few (if any?) non-Parent-but-CategoryObject-that-needs-base (or gens) classes, we can see how many there are and if we just want them to have a common API or a common ABC of CategoryObjectWithGens.

videlec commented 6 years ago

comment:30

I don't really care if we have to do a big change if it makes things better. Though we have to be sure that it will end with something better!

Is speed really a serious issue for accessing base or gens? This is indeed the whole problem of categories:

the category mantra says that a parent should just inherit from Parent (or possibly CategoryObject)
if we do that, the methods inherited from ParentMethods of the category are slow so we want custom base classes

jdemeyer commented 6 years ago

comment:31

Replying to @tscrim:

I am starting to wonder if we do want to actually have (new-style) Parent, ParentWithBase, ParentWithGens, and possibly ParentWithBaseGens classes that give these various features as common ABCs.

What do you mean exactly? Do you want to remove Parent? Do you want to remove Module? I am not following...

jdemeyer commented 6 years ago

comment:32

Replying to @videlec:

Is speed really a serious issue for accessing base?

One thing that I have been wanting to do since a long time is improving the semantics of _lmul_ and _rmul_ (which are meant for scalar multiplication). Most of the arithmetic/coercion model has been cleaned up by now, but those two methods are an ugly remaining corner. I would like to define _lmul_ and _rmul_ as multiplication by an element of parent._base. Ideally, that would have fast access to the _base attribute.

videlec commented 6 years ago

comment:33

Replying to @jdemeyer:

Replying to @videlec:

Is speed really a serious issue for accessing base?

One thing that I have been wanting to do since a long time is improving the semantics of _lmul_ and _rmul_ (which are meant for scalar multiplication). Most of the arithmetic/coercion model has been cleaned up by now, but those two methods are an ugly remaining corner. I would like to define _lmul_ and _rmul_ as multiplication by an element of parent._base. Ideally, that would have fast access to the _base attribute.

Once more, _base means "base ring on a module"! Would be better to use the name _base_ring and mention that it will be used for scalar multiplication. That would be a clear semantic.

jdemeyer commented 6 years ago

comment:34

Replying to @videlec:

Once more, _base means "base ring on a module"!

For this purpose, it certainly does.

Would be better to use the name _base_ring

Not sure because I wouldn't to require that it's a ring. I am sure that there are structures which admit some kind of scalar multiplication by a non-ring.

and mention that it will be used for scalar multiplication. That would be a clear semantic.

Sure! But then I would still like to keep _base on CategoryObject. The methods base() and base_ring() are a different story. Those might be implemented in the category.

videlec commented 6 years ago

comment:35

Replying to @jdemeyer:

Replying to @videlec:

Once more, _base means "base ring on a module"!

For this purpose, it certainly does.

Would be better to use the name _base_ring

Not sure because I wouldn't to require that it's a ring. I am sure that there are structures which admit some kind of scalar multiplication by a non-ring.

Fair enough. Examples? Let us keep _base as it used to be. But then specify that it is used precisely for _lmul_ and _rmul_ where the attribute declaration is.

and mention that it will be used for scalar multiplication. That would be a clear semantic.

Sure! But then I would still like to keep _base on CategoryObject. The methods base() and base_ring() are a different story. Those might be implemented in the category.

Why CategoryObject and not Parent?

Moving base() and base_ring() to the category would not be possible if the attribute _base is private. How do you plan to declare it?

tscrim commented 6 years ago

comment:36

Replying to @jdemeyer:

Replying to @tscrim:

I am starting to wonder if we do want to actually have (new-style) Parent, ParentWithBase, ParentWithGens, and possibly ParentWithBaseGens classes that give these various features as common ABCs.

What do you mean exactly? Do you want to remove Parent? Do you want to remove Module? I am not following...

I was thinking of a hierarchy like this:

+-Parent
|
+---ParentWithBase  (maybe useless right now)
|
+-+-ParentWithGens
  |
  +-+-ParentWithBaseAndGens
    |
    +-Module (etc).

with Parent being a subclass of CategoryObject, and we implement base(ring) and gens in the appropriate subclass. (I said new-style because there still is the old-style ParentWithGens floating around.) It is a bit more of a maintenance burden in terms of the class hierarchy, but it makes the code have better associations (as well as cleaning our user visible attributes).

jdemeyer commented 6 years ago

comment:37

Replying to @videlec:

Replying to @jdemeyer:

Replying to @videlec:

Once more, _base means "base ring on a module"!

For this purpose, it certainly does.

Would be better to use the name _base_ring

Not sure because I wouldn't to require that it's a ring. I am sure that there are structures which admit some kind of scalar multiplication by a non-ring.

Fair enough. Examples?

I don't know any example. It's just that there is no reason to force this to be a ring. Because then we have to worry about checking that _base_ring really is a ring. And if you don't check, then why call it _base_ring and not _base?

tscrim commented 6 years ago

comment:38

Replying to @videlec:

Is speed really a serious issue for accessing base or gens?

Getting the base ring and the generators are very common operations, so a slowdown in that access could potentially have much larger speed implications.

This is indeed the whole problem of categories:

the category mantra says that a parent should just inherit from Parent (or possibly CategoryObject)

if we do that, the methods inherited from ParentMethods of the category are slow so we want custom base classes

There usually should not be too many simple getter methods in a category that are used in tight loops that this is a problem, but there are a few that do warrant the special attention.

jdemeyer commented 6 years ago

comment:39

Replying to @videlec:

Moving base() and base_ring() to the category would not be possible if the attribute _base is private. How do you plan to declare it?

You could make _base accessible to Python code (cdef public _base instead of cdef _base).

jdemeyer commented 6 years ago

comment:40

Replying to @tscrim:

I was thinking of a hierarchy like this:

+-Parent
|
+---ParentWithBase  (maybe useless right now)
|
+-+-ParentWithGens
  |
  +-+-ParentWithBaseAndGens
    |
    +-Module (etc).

I don't think that it makes much sense to have unrelated classes ParentWithBase and ParentWithBaseAndGens. Since base seems more important than gens, I would actually suggest

CategoryObject
|
+--Parent
   |
   +--ParentWithBase
      |
      +--Module
      |
      +--Ring
      |
     ...

I said new-style because there still is the old-style ParentWithGens floating around.

Thanks for the reminder. Getting rid of those old-style parents should probably be higher priority than this ticket.

tscrim commented 6 years ago

comment:41

Replying to @jdemeyer:

Replying to @tscrim:
I was thinking of a hierarchy like this:
+-Parent
|
+---ParentWithBase  (maybe useless right now)
|
+-+-ParentWithGens
  |
  +-+-ParentWithBaseAndGens
    |
    +-Module (etc).
I don't think that it makes much sense to have unrelated classes ParentWithBase and ParentWithBaseAndGens.

Ideally it should be a diamond IMO, but Cython does not (yet?) support multiple inheritance (or something like interfaces in Java-speak).

Since base seems more important than gens, I would actually suggest
CategoryObject
|
+--Parent
   |
   +--ParentWithBase
      |
      +--Module
      |
      +--Ring
      |
     ...

We have objects with base but without gens (e.g., polytopes), objects without base but with gens (e.g., permutation groups), as well as with(out) both. So I was trying to address that with my proposed hierarchy. At least I would like ParentWithGens in between Parent and ParentWithBase as there are very few things in Sage that have gens but no concept of base. (Which, as it turns out, is not quite the old parent hierarchy ParentWithGens -> ParentWithBase -> Parent_old.)

Although I'm a little worried with the group class hierarchy becoming a bit more of a mess because of the Cython parents and not having the multiple inheritance. Might be a necessary complication in order to improve the interface (which IMO is net benefit). Spreading out to a more refined parent hierarchy is going to be a fairly invasive change, so I guess it will just have to be done.

I said new-style because there still is the old-style ParentWithGens floating around.

Thanks for the reminder. Getting rid of those old-style parents should probably be higher priority than this ticket.

Yes, definitely. Although most of them are really fundamental parents such as Ring, number fields, etc. I guess we can take care of the leaf classes a little more easily, such as ComplexIntervalField. Although it is somewhat counter to the objective of having a more refined parent hierarchy as it usually means making the class inherit from Parent...

jdemeyer commented 6 years ago

comment:42

Replying to @tscrim:

Cython does not (yet?) support multiple inheritance

First of all, the next release of Cython is going to support multiple inheritance in a limited way. The list of bases can be CyClass, PyClass1, ..., PyClassN, which is the most general thing which is allowed by Python and which doesn't hurt Cython's performance (because all Python classes come after all Cython classes in the MRO).

Second, the fact that diamonds are not allowed is because of Python's design. In plain Python, you cannot do class X(list, str) either. So don't blame Cython for something that they cannot fix.

Anyway, the conclusion is that ParentWithGens could be a Python class and then the diamond would work in Cython 0.28.

jdemeyer commented 6 years ago

comment:43

Replying to @tscrim:

Yes, definitely. Although most of them are really fundamental parents such as Ring

The large majority of rings use the new coercion model, so it's not as bad as you think.

tscrim commented 6 years ago

comment:44

Replying to @jdemeyer:

Replying to @tscrim:

Cython does not (yet?) support multiple inheritance

First of all, the next release of Cython is going to support multiple inheritance in a limited way. The list of bases can be CyClass, PyClass1, ..., PyClassN, which is the most general thing which is allowed by Python and which doesn't hurt Cython's performance (because all Python classes come after all Cython classes in the MRO).

That's good to hear and should be useful here.

Second, the fact that diamonds are not allowed is because of Python's design. In plain Python, you cannot do class X(list, str) either. So don't blame Cython for something that they cannot fix.

Sorry, I guess I was thinking a little too loosely with not distinguishing between classes and (builtin) types.

Anyway, the conclusion is that ParentWithGens could be a Python class and then the diamond would work in Cython 0.28.

I think this would be the most sensible solution since most of our Cython parent classes would inherit from the ParentWithGensAndBase (e.g., Ring, where we really treat everything as some R-algebra), but would keep the flexibility for other parents in a reasonable way.

tscrim commented 6 years ago

comment:45

Replying to @jdemeyer:

Replying to @tscrim:

Yes, definitely. Although most of them are really fundamental parents such as Ring

The large majority of rings use the new coercion model, so it's not as bad as you think.

That's good to hear. Now if only I could find some time to work on this directly...

videlec commented 6 years ago

Description changed:

--- 
+++ 
@@ -50,6 +50,8 @@

 - gradual transition for to remove inheritance from `ParentWithGens` to `Ring` (#13683? still useful?)

+- #24430: normalization for `_names`, `variable_name` and `variable_names` in the context of polynomials and series
+
 ### Transition to `FiniteEnumeratedSet`

 - set the category to `FiniteEnumeratedSets` where appropriate (#12957)

mkoeppe commented 3 years ago

comment:48

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.