Wir haben viel gelacht.

So, the problem is that Rings.ParentMethods.is_field checks whether self is the zero ring, which is not a field.

In the case at hand, this triggers the computation of 1, which in turn creates the default conversion map before the coercion was registered.

The reason why is_field was called is because the homset of the cyclotomic number field is eventually determined.

One solution is not to special case whether the ring is the zero ring, which should not happen to often anyway.

More generally, it seems to me that we should simply not try to deal with any obscure special cases, but simply require that every ring implements is_field.

Documentation preview for this PR (built with commit 75d4491; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Cool! The failure

sage -t --warn-long 5.0 --random-seed=96072386737223078858020577021659543344 src/sage/matrix/special.py
**********************************************************************
File "src/sage/matrix/special.py", line 3115, in sage.matrix.special.random_diagonalizable_matrix
Failed example:
    all(x in ZZ for x in (B-(4*identity_matrix(6))).rref().list())
Expected:
    True
Got:
    False

is real, but unrelated, it is the specific seed that triggers it.

What is quite surprising to me: the only instance where a coercion is registered after a conversion was discovered is the one we found!

@tscrim, is it hard to replace CellularBasis.one, i.e.,

    @cached_method
    def one(self):
        """
        Return the element `1` in ``self``.

        EXAMPLES::

            sage: S = SymmetricGroupAlgebra(QQ, 3)
            sage: C = S.cellular_basis()
            sage: C.one()
            C([1, 1, 1], [[1], [2], [3]], [[1], [2], [3]])
             + C([2, 1], [[1, 2], [3]], [[1, 2], [3]])
             + C([2, 1], [[1, 3], [2]], [[1, 3], [2]])
             + C([3], [[1, 2, 3]], [[1, 2, 3]])
        """
        return self(self._algebra.one())

by something that does not involve conversion?

If so, I see three possibilities to proceed:

• do not test for the zero ring in the generic is_field method, and possibly make the generic method always throw NotImplementedError (which amounts to saying that the ring is not a field)
• somehow remove the cached conversion before registering the coercion
• rewrite CellularBasis.one so that it does not invoke conversion.

Basically it is impossible. This type of construction should be allowed anyways. I don't quite understand your analysis at this point, so I can't offer any advice. I will try to look at it this tonight or tomorrow, but it would be helpful if you could expand a bit on what the problem is.

Hi @tscrim! Here is what I believe to understand. Consider the following setup:

sage: C5.<z5> = CyclotomicField(5)
sage: TL = TemperleyLiebAlgebra(2, z5 + ~z5, C5)
sage: B = TL.cellular_basis()
sage: isinstance(B, Ring)
False

We want to move is_field from Ring (in rings/ring.pyx) to Rings (in categories/rings.py). As a side effect, this method will be available to B. The initialisation of CellularBasis is as follows:

    def __init__(self, A, to_algebra=None, from_algebra=None, **kwargs):
...
        cat = Algebras(A.category().base_ring()).FiniteDimensional().WithBasis().Cellular()
        CombinatorialFreeModule.__init__(self, A.base_ring(), I,
                                         prefix=prefix, bracket=False,
                                         category=cat, **kwargs)
...
        to_cellular.register_as_coercion()
        from_cellular.register_as_coercion()

After this PR, the CombinatorialFreeModule.__init__ eventually triggers the execution of Rings.ParentMethods.is_field, which in turn triggers the execution of self.one(). This in turn has the effect that a conversion from B._algebra (which is just TL) to B is discovered:

sage: B.convert_map_from(TL)
Conversion map:
  From: Temperley-Lieb Algebra of rank 2 with parameter -z5^3 - z5^2 - 1 over Cyclotomic Field of order 5 and degree 4
  To:   Cellular basis of Temperley-Lieb Algebra of rank 2 with parameter -z5^3 - z5^2 - 1 over Cyclotomic Field of order 5 and degree 4
sage: type(B.convert_map_from(TL))
<class 'sage.structure.coerce_maps.DefaultConvertMap_unique'>

However, this map is apparently the wrong one. In fact, it is unable to compute B.one() - the computation fails with a TypeError, but the error is caught (I think in categories/homset.py line 445, which is part of the definition of Hom).

Does this help?

Interesting. This is something specific when the base ring is a number field. It comes from the number field testing the field-ness of the codomain for the base ring morphism. This returns False (of course, well...in the general case), which then makes the homset be the generic version. This seems a bit dangerous, and to fix this, the _Hom_ should consider the category which we are considering the objects (which I think should be done anyways). So this fixes the issue for me:

diff --git a/src/sage/rings/number_field/number_field.py b/src/sage/rings/number_field/number_field.py
index 68031c3d642..d58dd8633a3 100644
--- a/src/sage/rings/number_field/number_field.py
+++ b/src/sage/rings/number_field/number_field.py
@@ -136,15 +136,18 @@ lazy_import('sage.rings.universal_cyclotomic_field', 'UniversalCyclotomicFieldEl
 _NumberFields = NumberFields()
 
 
-def is_NumberFieldHomsetCodomain(codomain):
+def is_NumberFieldHomsetCodomain(codomain, category=None):
     """
-    Return whether ``codomain`` is a valid codomain for a number
-    field homset.
+    Return whether ``codomain`` is a valid codomain for a number field
+    homset (as an object in the category ``category`` if specified).
 
     This is used by NumberField._Hom_ to determine
     whether the created homsets should be a
     :class:`sage.rings.number_field.homset.NumberFieldHomset`.
 
+    This also takes into account the category, which checks if it is
+    a subcategory of :class:`Fields`.
+
     EXAMPLES:
 
     This currently accepts any parent (CC, RR, ...) in :class:`Fields`::
@@ -173,7 +176,9 @@ def is_NumberFieldHomsetCodomain(codomain):
         False
     """
     from sage.categories.fields import Fields
-    return codomain in Fields()
+    if category is None:
+        return codomain in Fields()
+    return category.is_subcategory(Fields())
 
 
 def proof_flag(t):
@@ -1988,7 +1993,7 @@ class NumberField_generic(WithEqualityById, number_field_base.NumberField):
             sage: loads(dumps(H)) is H
             True
         """
-        if not is_NumberFieldHomsetCodomain(codomain):
+        if not is_NumberFieldHomsetCodomain(codomain, category):
             # Using LazyFormat fixes #28036 - infinite loop
             from sage.misc.lazy_format import LazyFormat
             raise TypeError(LazyFormat("%s is not suitable as codomain for homomorphisms from %s") % (codomain, self))
@@ -11476,7 +11481,7 @@ class NumberField_cyclotomic(NumberField_absolute, sage.rings.abc.NumberField_cy
             sage: End(CyclotomicField(21))
             Automorphism group of Cyclotomic Field of order 21 and degree 12
         """
-        if is_NumberFieldHomsetCodomain(codomain):
+        if is_NumberFieldHomsetCodomain(codomain, cat):
             from sage.rings.number_field.homset import CyclotomicFieldHomset
             return CyclotomicFieldHomset(self, codomain)
         else:

Let me put it another way, I think it is bad to have self.is_field() called during initialization because self might not necessarily fully initialized (as evidenced by this bug).

Let me put it another way, I think it is bad to have self.is_field() called during initialization because self might not necessarily fully initialized (as evidenced by this bug).

I agree, but it is not clear to me how to avoid that properly. In the case at hand, it is calling self.one() that fails, because that's only available after initialization. However, accessing self.one() in the category code seems hard to forbid, no?

We can’t enforce it, but neither can we enforce people from doing other evil things with code (other than with careful reviewing and having good doctests). I think it is fairly rare that something like this will happen; in this case it is somewhat of a coincidence as the map already knows it is not a map as fields (in general). (Side note: it might be better for AlgebrasWithBasis to check is_zero by dimension like ModulesWithBasis.) I think if this comes up again in the future, we can revisit my solution (and the rest of the interrelationships) then. However, this makes the map and homset behave at the level that the category code is basically expecting, so it feels like the correct solution to me.

@tscrim I don't fully understand your proposed change but maybe I am misunderstanding how things are supposed to work.

So, when I say Hom(X, Y, cat) what should be the category where these morphisms live? Should it be the morphisms of cat? Or should it be the morphisms of a subcategory of cat, probably the morphisms of a subcategory if it is a fully faithful embedding? (Currently, it seems to try to be the latter and I guess the docstring of Hom could be a bit more clear about this.) (Bear with my limited knowledge of category theory, I hope I used the terminology correctly.)

That said, it's unclear what is_NumberFieldHomsetCodomain() is supposed to do, not even the docstring knows:

Question: should, for example, QQ-algebras be accepted as well?

It seems odd to me that a function by that names looks at a category argument and not at the codomain at all.

So, what this function is really about is whether NumberFieldHomset (and its subclasses) should be used. These classes say that they are ring homsets with the domain a number field. So, shouldn't we just test codomain in Rings() and cat a subcategory of rings in the first place? (Of course, these homsets are often empty, there's no ring hom from QQ to ZZ but there's also no such morphism from QQ(sqrt(2)) to QQ.)

Here's a different proposal. I played a bit with it locally, and it doesn't seem to be obviously broken. But maybe lots of doctests fail now. Let's see…

This is independent of my latest change, but I don't think this is good:

 sage: K = CyclotomicField(3)
 sage: Hom(K, ZZ).category()
-Category of homsets of euclidean domains and noetherian rings
+Category of homsets of unital magmas and additive unital additive magmas

Fixed in 8715d39.

Are we to blame for that?

sage -t --long --warn-long 30.0 --random-seed=286735480429121101562228604801325644303 src/sage/rings/polynomial/polynomial_element.pyx  # Timed out

Tests pass for me locally. Let's see if the CI is happy if we merge in dev again.

sage -t --warn-long 5.0 --random-seed=30791175965571573276774496272530939944 src/sage/modules/free_module_pseudomorphism.py  # 1 doctest failed

i.e.

 File "src/sage/modules/free_module_pseudomorphism.py", line 483, in sage.modules.free_module_pseudomorphism.FreeModulePseudoMorphism.__eq__
Failed example:
    f == g
Expected:
    False
Got:
    True

I doubt we have anything to do with that.

This doctest in src/sage/modules/free_module_pseudomorphism also fails in vanilla sage, so it's not related to what we do here.

I have re-launched the CI. Not sure what is blocking us here now.

I have re-launched the CI. Not sure what is blocking us here now.

You mean why the CI fails? I don't know. If you mean why this does not have positive review? I think we should set this to positive review.

I think this is ready, isn't it?

I think so. But let me first re-run the tests by merging with the latest beta.

ok, this looks good enough. I am setting to positive.