[WIP] Always use dynamic classes for elements

[jdemeyer]

Use dynamic classes, both for Python and Cython classes.

CC: @nthiery @tscrim

Component: categories

Author: Jeroen Demeyer

Branch/Commit: u/jdemeyer/always_use_dynamic_classes @ ebee325

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

[jdemeyer]

Description changed:

--- 
+++ 
@@ -1 +1 @@
-
+Use dynamic classes, both for Python and Cython classes.

[jdemeyer]

Branch: u/jdemeyer/always_use_dynamic_classes

[jdemeyer]

Commit: ebee325

[jdemeyer]

comment:3

This is the basic idea. But this leads to a million (well, a lot) doctest failures of the form

sage -t src/sage/rings/finite_rings/element_pari_ffelt.pyx
**********************************************************************
File "src/sage/rings/finite_rings/element_pari_ffelt.pyx", line 100, in sage.rings.finite_rings.element_pari_ffelt.FiniteFieldElement_pari_ffelt
Failed example:
    type(a)
Expected:
    <type 'sage.rings.finite_rings.element_pari_ffelt.FiniteFieldElement_pari_ffelt'>
Got:
    <class 'sage.rings.finite_rings.finite_field_pari_ffelt.FiniteField_pari_ffelt_with_category.element_class'>
**********************************************************************

These are clearly desired doctest failures.

Second, many parents still hardcode the element type. For example, even with this patch:

sage: type(ZZ.one())
<type 'sage.rings.integer.Integer'>
sage: type(RR.one())
<type 'sage.rings.real_mpfr.RealNumber'>

Fixing those is obviously outside the scope of this ticket.

---

New commits:

ebee325

Always use dynamic classes

[jdemeyer]

comment:4

Setting to needs_review just for the patchbot.

[jdemeyer]

comment:6

Interesting fact: combinat already uses dynamic classes for almost everything. Only doctest failures in combinat:

sage -t src/sage/combinat/crystals/tensor_product_element.pyx
**********************************************************************
File "src/sage/combinat/crystals/tensor_product_element.pyx", line 752, in sage.combinat.crystals.tensor_product_element.CrystalOfTableauxElement.__init__
Failed example:
    type(t[0])
Expected:
    <... 'sage.combinat.crystals.letters.Crystal_of_letters_type_A_element'>
Got:
    <class 'sage.combinat.crystals.letters.ClassicalCrystalOfLetters_with_category.element_class'>
**********************************************************************
File "src/sage/combinat/crystals/tensor_product_element.pyx", line 755, in sage.combinat.crystals.tensor_product_element.CrystalOfTableauxElement.__init__
Failed example:
    type(t[1])
Expected:
    <... 'sage.combinat.crystals.letters.Crystal_of_letters_type_A_element'>
Got:
    <class 'sage.combinat.crystals.letters.ClassicalCrystalOfLetters_with_category.element_class'>
**********************************************************************

[tscrim]

comment:7

From a quick look at the current patchbot reports:

• There seems to be a very non-trivial side effect when wrapping Integer (see the categories/examples/infinite_enumerated_sets.py).
• The failures in categories/sets_cat.py and sets/cartesian_product.py I believe are connected with the previous point.
• We are getting TypeErrors being raised in modular/btquotients/pautomorphicform.py.

• Coercion has broken in rings/polynomial/polynomial_ring.py coming from this check:

                  # Over ZZ, only allow coercion from any ZZ['x']
                  # implementation to the default FLINT implementation
                  if self.element_class is not Polynomial_integer_dense_flint:
                      return None

  It would need to become issubclass, which means a slight speed penalty, but a necessary one.

• I think the failures in libs/coxeter3/coxeter_group.py and libs/coxeter3/coxeter.pyx are from this ticket, but I haven't verified locally.
• This uncovered a bad import: A doctest in structure.misc is importing is_extension_type from structure.parent: from sage.structure.parent import is_extension_type.
• Something is going wrong in structure/list_clone.pyx with pickling.

The rest look like trivial failures a la comment:3.

I am also somewhat worried about speed regressions overall.

[nbruin]

comment:9

I think this would cause speed regression in every cpdef call. Whenever a type is subclasses, cpdef must take the slow codepath in order to check if it's been overridden. I haven't checked if cython actually does that, but this should even be the case if the subclass is an extension type, since the cpdef on the super doesn't know what kind of shenanigans have been played (the subtype might have grown an instance dict and the method might be overridden by a function in there).

Definitely worth checking!

It may be worth saying something about the benefits of making this chance, so that those can be weighed against the draw-backs.

[jdemeyer]

comment:10

Replying to @nbruin:

I think this would cause speed regression in every cpdef call.

That is why I created #27090: Cython only checks for cpdef overrides in Python classes.

[nbruin]

comment:11

Replying to @jdemeyer:

That is why I created #27090: Cython only checks for cpdef overrides in Python classes.

I figured as much. Do make sure to include a test that confirms this behaviour, because if you'd rely on this, you'd be relying in a flaw in cython that is liable to be fixed in a future version. Given the positive side-effect of the flaw, I can imagine that the cython devs would be willing to include a pragma to revert to the old behaviour if it comes to that, but we'd have to know to prod them for it.

If we don't test it we'll probably suffer a hard to measure but possibly in some cases significant performance regression with some future cython upgrade.

[jdemeyer]

comment:12

Replying to @nbruin:

If we don't test it we'll probably suffer a hard to measure but possibly in some cases significant performance regression with some future cython upgrade.

If it helps: I discussed this with the Cython devs in person at an OpenDreamKit workshop and the current behavior was a deliberate choice, not an implementation accident which will gratuitously be changed. Of course, as usual there are no guarantees...