Remove RingHomomorphism_coercion

[saraedum]

we should try to use .coerce_map_from() instead, so we get is_injective()/is_surjective() and friends right for Hom.natural_map().

[git]

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

​2043ff3

Fixing doctest errors

[git]

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

​d7eb179	Remove RingHomomorphism_coercion
​dbd5b0d	fix morphism printing in doctests
​f606020	Fixed some doctests

[git]

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

​db5cee6

implement zero() for ring homsets

[git]

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

​a42ebcf

implement __nonzero__ when there is no zero element

[saraedum]

New commits:

​a42ebcf

implement __nonzero__ when there is no zero element

---

New commits:

​a42ebcf

implement __nonzero__ when there is no zero element

[saraedum]

The speed regression is minimal. About 235ns vs 220ns in some constructed cases. It should not have much of an effect since most elements overwrite __nonzero__ anyway.

[xander.faber]

Doctests are failing in structure/element.pyx, rings/homset.py, combinat/combinat.py, and rings/fraction_field.FpT.pyx. This has to do with the Element.__nonzero__() method producing behavior that disagrees with its docstring. I am addressing this currently.

[tscrim]

A strong -1 on calling the category (first): this is relatively expensive, not likely able to be implemented by the MorphismMethods of a category if you have to do formal compositions, and is meant to be overwritten. Really, if you want composite maps, then you should overwrite the _composition_ hook.

No bare exceptions.

I don't understand why this needs to be a more generic class:

src/sage/rings/morphism.pxd

@@ -15 +15 @@
     cdef CategoryObject S
 
 cdef class RingHomomorphism(RingMap):
-    cdef RingMap _lift
-
-cdef class RingHomomorphism_coercion(RingHomomorphism):
-    pass
+    cdef Morphism _lift
 
 cdef class RingHomomorphism_im_gens(RingHomomorphism):
     cdef __im_gens

This is almost certainly a speed regression:

src/sage/rings/morphism.pyx

@@ -373 +375 @@
         sage: sage.rings.morphism.is_RingHomomorphism(2/3)
         False
     """
-    return isinstance(phi, RingHomomorphism)
+    # We use the category framework to determine whether something is a ring homomorphism.
+    from sage.categories.map import Map
+    from sage.categories.all import Rings
+    return isinstance(phi, Map) and phi.category_for().is_subcategory(Rings())
 
 cdef class RingMap(Morphism):
     """

This may not be so important as it may not really be used in time-critical code, but it shows up all over the schemes stuff (which should be replaced with a direct isinstance call as they know the morphisms being construction are a subclass of RingHomomorphism). I understand why this is natural, but I don't think this should be done (here) as it is a true behavior change, the is_* functions use are discouraged for trivial cases, and the the above.

I am a little wary of removing RingHomomorphism.__nonzero__ as that specialized code does do a check that could be faster and might be a little better behaved because no equality checking is involved. I just want to make sure you really checked this doesn't introduce a speed regression and works well for inexact rings (which may not be fully doctested).

Why is this change done:

src/sage/schemes/generic/morphism.py

@@ -1442 +1441 @@
             S = self.codomain().change_ring(R)
             H = Hom(T,S)
 
-        if isinstance(R, Morphism):
+        if isinstance(R, Map):
             if R.domain() == self.base_ring():
                 R = self.domain().ambient_space().coordinate_ring().hom(R, T.ambient_space().coordinate_ring())
         G = []

It does not immediately seem to be related to this ticket.

You need to properly handle old pickles with the old class.

Error messages are not really sentences (according to Python's conventions):

-raise TypeError("Natural coercion morphism from %s to %s not defined."%(self.domain(), self.codomain()))
+raise TypeError("natural coercion morphism from %s to %s not defined"%(self.domain(), self.codomain()))

This change really makes me worry:

@@ -737,11 +715,18 @@ cdef class RingHomomorphism(RingMap):
             sage: f = R.hom([a+b,a-b])
             sage: g = S.hom(Frac(S))
             sage: g*f # indirect doctest
-            Ring morphism:
+            Composite map:
               From: Multivariate Polynomial Ring in x, y over Rational Field
               To:   Fraction Field of Multivariate Polynomial Ring in a, b over Rational Field
-              Defn: x |--> a + b
-                    y |--> a - b
+              Defn:   Ring morphism:
+                      From: Multivariate Polynomial Ring in x, y over Rational Field
+                      To:   Multivariate Polynomial Ring in a, b over Rational Field
+                      Defn: x |--> a + b
+                            y |--> a - b
+                    then
+                      Conversion via FractionFieldElement map:
+                      From: Multivariate Polynomial Ring in a, b over Rational Field
+                      To:   Fraction Field of Multivariate Polynomial Ring in a, b over Rational Field
 
         When ``right`` is defined by the images of generators, the
         result has the type of a homomorphism between its domain and

In particular, it seems like you will now get coercion morphisms that are compositions, which seems to be different behavior than before. This could have subtle effects on code and likely have a performance regression.

I agree with the purpose of this ticket and would like to see the ring morphism code simplified. However, I care a lot about the speed in here because polynomials are a core part of the Sage functionality.

[saraedum]

David Roe and I looked at your comments.

Replying to tscrim:

A strong -1 on calling the category (first): this is relatively expensive, not likely able to be implemented by the MorphismMethods of a category if you have to do formal compositions, and is meant to be overwritten. Really, if you want composite maps, then you should overwrite the _composition_ hook.

We don't understand what you are trying to say here. Could you rephrase your concern please.

No bare exceptions.

What do you mean?

I don't understand why this needs to be a more generic class:

src/sage/rings/morphism.pxd

@@ -15 +15 @@
     cdef CategoryObject S
 
 cdef class RingHomomorphism(RingMap):
-    cdef RingMap _lift
-
-cdef class RingHomomorphism_coercion(RingHomomorphism):
-    pass
+    cdef Morphism _lift
 
 cdef class RingHomomorphism_im_gens(RingHomomorphism):
     cdef __im_gens

Because some lift map is now not inheriting from RingMap anymore.

This is almost certainly a speed regression:

src/sage/rings/morphism.pyx

@@ -373 +375 @@
         sage: sage.rings.morphism.is_RingHomomorphism(2/3)
         False
     """
-    return isinstance(phi, RingHomomorphism)
+    # We use the category framework to determine whether something is a ring homomorphism.
+    from sage.categories.map import Map
+    from sage.categories.all import Rings
+    return isinstance(phi, Map) and phi.category_for().is_subcategory(Rings())
 
 cdef class RingMap(Morphism):
     """

This may not be so important as it may not really be used in time-critical code, but it shows up all over the schemes stuff (which should be replaced with a direct isinstance call as they know the morphisms being construction are a subclass of RingHomomorphism). I understand why this is natural, but I don't think this should be done (here) as it is a true behavior change, the is_* functions use are discouraged for trivial cases, and the the above.

Ok. Let us try to take the is_RingHomomorphism out completely.

I am a little wary of removing RingHomomorphism.__nonzero__ as that specialized code does do a check that could be faster and might be a little better behaved because no equality checking is involved. I just want to make sure you really checked this doesn't introduce a speed regression and works well for inexact rings (which may not be fully doctested).

We think that you will never get to the == check in the new implementation in the case of ring homomorphism (unless the codomain is trivial). We checked a few cases and there was no speed regression.

Why is this change done:

src/sage/schemes/generic/morphism.py

@@ -1442 +1441 @@
             S = self.codomain().change_ring(R)
             H = Hom(T,S)
 
-        if isinstance(R, Morphism):
+        if isinstance(R, Map):
             if R.domain() == self.base_ring():
                 R = self.domain().ambient_space().coordinate_ring().hom(R, T.ambient_space().coordinate_ring())
         G = []

It does not immediately seem to be related to this ticket.

The reason is that default convert maps do not inherit from Morphism. It is certainly related to this ticket. The type of a map changed, so thi isinstance check has to be more liberal.

You need to properly handle old pickles with the old class.

Sure. The patchbot already complains about this.

Error messages are not really sentences (according to Python's conventions):

-raise TypeError("Natural coercion morphism from %s to %s not defined."%(self.domain(), self.codomain()))
+raise TypeError("natural coercion morphism from %s to %s not defined"%(self.domain(), self.codomain()))

We do not understand what is the change that you are proposing.

This change really makes me worry:

@@ -737,11 +715,18 @@ cdef class RingHomomorphism(RingMap):
             sage: f = R.hom([a+b,a-b])
             sage: g = S.hom(Frac(S))
             sage: g*f # indirect doctest
-            Ring morphism:
+            Composite map:
               From: Multivariate Polynomial Ring in x, y over Rational Field
               To:   Fraction Field of Multivariate Polynomial Ring in a, b over Rational Field
-              Defn: x |--> a + b
-                    y |--> a - b
+              Defn:   Ring morphism:
+                      From: Multivariate Polynomial Ring in x, y over Rational Field
+                      To:   Multivariate Polynomial Ring in a, b over Rational Field
+                      Defn: x |--> a + b
+                            y |--> a - b
+                    then
+                      Conversion via FractionFieldElement map:
+                      From: Multivariate Polynomial Ring in a, b over Rational Field
+                      To:   Fraction Field of Multivariate Polynomial Ring in a, b over Rational Field
 
         When ``right`` is defined by the images of generators, the
         result has the type of a homomorphism between its domain and

In particular, it seems like you will now get coercion morphisms that are compositions, which seems to be different behavior than before. This could have subtle effects on code and likely have a performance regression.

David is going to have another look. There should not be a speed regression. Actually the opposite should be the case. Before the composite map was wrapped in another map, now it is directly accessible. We are going to double check that this is true.

I agree with the purpose of this ticket and would like to see the ring morphism code simplified. However, I care a lot about the speed in here because polynomials are a core part of the Sage functionality.

Sure. Let's be careful about not causing any regressions here.

---

New commits:

​a810e3c	Fixed Element.__nonzero__ and doctests
​96b2d87	Fixed doctest for rings.homset.RingHomset_generic.zero
​e11de69	Fixed doctests for FpT_Polyring_section and FpT_Fp_section
​83a83dd	Fixed doctests for CombinatorialObject.__bool__
​6ddff36	Merge branch 'develop' into t/23204/remove_ringhomomorphism_coercion

[tscrim]

Replying to saraedum:

David Roe and I looked at your comments.

Replying to tscrim:

A strong -1 on calling the category (first): this is relatively expensive, not likely able to be implemented by the MorphismMethods of a category if you have to do formal compositions, and is meant to be overwritten. Really, if you want composite maps, then you should overwrite the _composition_ hook.

We don't understand what you are trying to say here. Could you rephrase your concern please.

You added two blocks for formal composite functions:

        try:
            # we try the category first
            # as of 2017-06, the MRO of this class does not get patched to
            # include the category's MorphismMethods (because it is a Cython
            # class); therefore, we can not simply call "super" but need to
            # invoke the category method explicitly
            return self.getattr_from_category('is_injective')()
        except (AttributeError, NotImplementedError):
            pass

What I am saying is this is really (relatively) expensive for something that is almost certainly going to be implemented. If your category has morphisms such that you cannot effectively simplify a composition f \circ g to a single map h, then I don't see how you would have enough (generic) information to check that the composition is injective/surjective. So I doubt this would ever get used.

In contrast, when a category does have enough information to simplify maps, it should implement the _composition_ hook to return the correct composition map.

No bare exceptions.

What do you mean?

You indirectly took care of it in a810e3c:

@@ -998,12 +998,16 @@ cdef class Element(SageObject):
             False
 
         """
+        if hasattr(self,'_parent'):
+            P = self._parent
+        elif hasattr(self,'parent'):
+            P = self.parent()
+        else:
+            return True # By convention!
         try:
-            zero = self._parent.zero()
-        except:
-            return False
-
-        return self != zero
+            return self != P.zero()
+        except (AttributeError,ValueError):
+            return True # By convention!
 
     def is_zero(self):
         """

Actually, I don't see how you could get anything other than the first case as Element has a _parent attribute this is checked within Element), much less into the second block because why would it have a meaningful .parent() method in that case? If there is such a class out there, then it is a bug IMO and should be fixed to initialize Element. So I would probably just do

try:
    return self != self._parent.zero()
except Exception:
    return True # By convention

unless you want something that returns a TypeError or NotImplementedError to not be caught. For that I am a little more unsure.

I don't understand why this needs to be a more generic class: [snip]

Because some lift map is now not inheriting from RingMap anymore.

That smells a little like you have encountered a bug and something really should be returning a RingMap_lift. Either that or we should also remove RingMap_lift with some more generic class for lifts (if that is what we decide to do, we can do that here or on a followup).

I am a little wary of removing RingHomomorphism.__nonzero__ as that specialized code does do a check that could be faster and might be a little better behaved because no equality checking is involved. I just want to make sure you really checked this doesn't introduce a speed regression and works well for inexact rings (which may not be fully doctested).

We think that you will never get to the == check in the new implementation in the case of ring homomorphism (unless the codomain is trivial). We checked a few cases and there was no speed regression.

That does not seem possible. If the ring homset has no zero, then is relatively expensive because of the throwing and catching of an error and is something done every time. If there is a zero map, then it should only affect the first time it is called, but then it has to do a comparison because of the default Element.__nonzero__. What were the cases you tested?

Why is this change done: [snip] It does not immediately seem to be related to this ticket.

The reason is that default convert maps do not inherit from Morphism. It is certainly related to this ticket. The type of a map changed, so thi isinstance check has to be more liberal.

I do not see why this needs to be reduced to Map but for RingMap, it can still be passed in a Morphism. If you're sure that this is okay, then I will take your word for it because I don't know that area of the codebase so well.

Error messages are not really sentences (according to Python's conventions):

-raise TypeError("Natural coercion morphism from %s to %s not defined."%(self.domain(), self.codomain()))
+raise TypeError("natural coercion morphism from %s to %s not defined"%(self.domain(), self.codomain()))

We do not understand what is the change that you are proposing.

You should use lower case for the initial letters and not have a period (full stop) at the end. Even if this differs with the rest of the file, we are trying to follow Python conventions.

This change really makes me worry: [snip] In particular, it seems like you will now get coercion morphisms that are compositions, which seems to be different behavior than before. This could have subtle effects on code and likely have a performance regression.

David is going to have another look. There should not be a speed regression. Actually the opposite should be the case. Before the composite map was wrapped in another map, now it is directly accessible. We are going to double check that this is true.

You should be correct as no parent implements a custom coerce. However, this does mean we have to be more liberal with types (see above).

Could you explain why you needed to change this doctest:

@@ -861,7 +808,7 @@ cdef class RingHomomorphism(RingMap):
 
         This is not implemented in any generality yet::
 
-            sage: f = ZZ.hom(ZZ)
+            sage: f = ZZ.hom(Zp)
             sage: f.inverse_image(ZZ.ideal(2))
             Traceback (most recent call last):
             ...

[saraedum]

Replying to tscrim:

Replying to saraedum:

David Roe and I looked at your comments.

Replying to tscrim:

A strong -1 on calling the category (first): this is relatively expensive, not likely able to be implemented by the MorphismMethods of a category if you have to do formal compositions, and is meant to be overwritten. Really, if you want composite maps, then you should overwrite the _composition_ hook.

We don't understand what you are trying to say here. Could you rephrase your concern please.

You added two blocks for formal composite functions:

        try:
            # we try the category first
            # as of 2017-06, the MRO of this class does not get patched to
            # include the category's MorphismMethods (because it is a Cython
            # class); therefore, we can not simply call "super" but need to
            # invoke the category method explicitly
            return self.getattr_from_category('is_injective')()
        except (AttributeError, NotImplementedError):
            pass

What I am saying is this is really (relatively) expensive for something that is almost certainly going to be implemented. If your category has morphisms such that you cannot effectively simplify a composition f \circ g to a single map h, then I don't see how you would have enough (generic) information to check that the composition is injective/surjective. So I doubt this would ever get used.

I do not understand how this relates to _composition_. The point here is simply that in the category of Rings, a morphism that has a field as its domain, knows that it is injective. However, we can not so easily turn the composition into one morphism.

No bare exceptions.

What do you mean?

You indirectly took care of it in a810e3c:

Ok.

Actually, I don't see how you could get anything other than the first case as Element has a _parent attribute this is checked within Element), much less into the second block because why would it have a meaningful .parent() method in that case? If there is such a class out there, then it is a bug IMO and should be fixed to initialize Element. So I would probably just do

try:
    return self != self._parent.zero()
except Exception:
    return True # By convention

unless you want something that returns a TypeError or NotImplementedError to not be caught. For that I am a little more unsure.

I agree. What were the elements without _parent?

I don't understand why this needs to be a more generic class: [snip]

Because some lift map is now not inheriting from RingMap anymore.

That smells a little like you have encountered a bug and something really should be returning a RingMap_lift. Either that or we should also remove RingMap_lift with some more generic class for lifts (if that is what we decide to do, we can do that here or on a followup).

Yes, we should remove RingMap_lift. I don't remember the details but changing this type was not a hack to get things to compile.

I am a little wary of removing RingHomomorphism.__nonzero__ as that specialized code does do a check that could be faster and might be a little better behaved because no equality checking is involved. I just want to make sure you really checked this doesn't introduce a speed regression and works well for inexact rings (which may not be fully doctested).

We think that you will never get to the == check in the new implementation in the case of ring homomorphism (unless the codomain is trivial). We checked a few cases and there was no speed regression.

That does not seem possible. If the ring homset has no zero, then is relatively expensive because of the throwing and catching of an error and is something done every time. If there is a zero map, then it should only affect the first time it is called, but then it has to do a comparison because of the default Element.__nonzero__. What were the cases you tested?

Sorry, I had misread your initial comment. There is probably a speed regression but when would this possibly be relevant (i.e., when do you create lots of rings and check that morphisms between them are nonzero? without knowing that you are talking about rings.)

sage: f = ZZ.hom(Zp(3))
sage: %timeit bool(f)
1000000 loops, best of 3: 212 ns per loop # before
10000 loops, best of 3: 25.5 µs per loop # after

Why is this change done: [snip] It does not immediately seem to be related to this ticket.

The reason is that default convert maps do not inherit from Morphism. It is certainly related to this ticket. The type of a map changed, so thi isinstance check has to be more liberal.

I do not see why this needs to be reduced to Map but for RingMap, it can still be passed in a Morphism. If you're sure that this is okay, then I will take your word for it because I don't know that area of the codebase so well.

Ok.

Error messages are not really sentences (according to Python's conventions):

-raise TypeError("Natural coercion morphism from %s to %s not defined."%(self.domain(), self.codomain()))
+raise TypeError("natural coercion morphism from %s to %s not defined"%(self.domain(), self.codomain()))

Fixed.

This change really makes me worry: [snip] In particular, it seems like you will now get coercion morphisms that are compositions, which seems to be different behavior than before. This could have subtle effects on code and likely have a performance regression.

David is going to have another look. There should not be a speed regression. Actually the opposite should be the case. Before the composite map was wrapped in another map, now it is directly accessible. We are going to double check that this is true.

You should be correct as no parent implements a custom coerce. However, this does mean we have to be more liberal with types (see above).

Yes.

Could you explain why you needed to change this doctest:

@@ -861,7 +808,7 @@ cdef class RingHomomorphism(RingMap):
 
         This is not implemented in any generality yet::
 
-            sage: f = ZZ.hom(ZZ)
+            sage: f = ZZ.hom(Zp)
             sage: f.inverse_image(ZZ.ideal(2))
             Traceback (most recent call last):
             ...

ZZ.hom(ZZ) is an IdentityMorphism now which does not have an inverse_image() method. We went from NotImplementedError to AttributeError it seems.

[git]

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

​cd600d8	fix typo
​b2f5e24	fix typo
​7eccce6	Merge branch 'develop' into t/23204/remove_ringhomomorphism_coercion
​24173a5	fix wording
​29bad40	Merge branch 'u/saraedum/remove_ringhomomorphism_coercion' of git://trac.sagemath.org/sage into t/23204/remove_ringhomomorphism_coercion

[tscrim]

Replying to saraedum:

Replying to tscrim:

Replying to saraedum:

David Roe and I looked at your comments.

Replying to tscrim:

A strong -1 on calling the category (first): this is relatively expensive, not likely able to be implemented by the MorphismMethods of a category if you have to do formal compositions, and is meant to be overwritten. Really, if you want composite maps, then you should overwrite the _composition_ hook.

We don't understand what you are trying to say here. Could you rephrase your concern please.

You added two blocks for formal composite functions: [snip] What I am saying is this is really (relatively) expensive for something that is almost certainly going to be implemented. If your category has morphisms such that you cannot effectively simplify a composition f \circ g to a single map h, then I don't see how you would have enough (generic) information to check that the composition is injective/surjective. So I doubt this would ever get used.

I do not understand how this relates to _composition_. The point here is simply that in the category of Rings, a morphism that has a field as its domain, knows that it is injective. However, we can not so easily turn the composition into one morphism.

So you're doing implementing that at the category level? I feel like for something like this you should have a generic field morphism class that knows outright that it is injective and implements a custom _composition_ that returns the same class because it is completely determined by the image of 1. So the composition would amount to chasing through each map what happens to 1. If you are using some more specialized morphisms and need the generic composition, then let the generic code of the composition do its thing. I don't see the point of slowing everything else down for a special case that is more effectively dealt with by a concrete class.

I agree. What were the elements without _parent?

My point is that never happens. All elements must inherit from Element (or you are basically reimplementing Element with less features).

I don't understand why this needs to be a more generic class: [snip]

Because some lift map is now not inheriting from RingMap anymore.

That smells a little like you have encountered a bug and something really should be returning a RingMap_lift. Either that or we should also remove RingMap_lift with some more generic class for lifts (if that is what we decide to do, we can do that here or on a followup).

Yes, we should remove RingMap_lift. I don't remember the details but changing this type was not a hack to get things to compile.

I would call it a temporary fix until we remove RingMap_list. However, will that be a followup or done here? There is good reason to do it here as we are already touching a lot of places and is similar in spirit, but it would be a definite expansion of scope. I leave it up to you.

I am a little wary of removing RingHomomorphism.__nonzero__ as that specialized code does do a check that could be faster and might be a little better behaved because no equality checking is involved. I just want to make sure you really checked this doesn't introduce a speed regression and works well for inexact rings (which may not be fully doctested).

We think that you will never get to the == check in the new implementation in the case of ring homomorphism (unless the codomain is trivial). We checked a few cases and there was no speed regression.

That does not seem possible. If the ring homset has no zero, then is relatively expensive because of the throwing and catching of an error and is something done every time. If there is a zero map, then it should only affect the first time it is called, but then it has to do a comparison because of the default Element.__nonzero__. What were the cases you tested?

Sorry, I had misread your initial comment. There is probably a speed regression but when would this possibly be relevant (i.e., when do you create lots of rings and check that morphisms between them are nonzero? without knowing that you are talking about rings.)

sage: f = ZZ.hom(Zp(3))
sage: %timeit bool(f)
1000000 loops, best of 3: 212 ns per loop # before
10000 loops, best of 3: 25.5 µs per loop # after

I don't have a use case for it, but I do not see the point in deliberately introducing a regression by removing code that is (was?) working perfectly well. Someone might have a use case for it; I know people create lots of polynomial rings over finite fields as the field size varies, and they might be doing a check for a nonzero map in that code.

Could you explain why you needed to change this doctest:

@@ -861,7 +808,7 @@ cdef class RingHomomorphism(RingMap):
 
         This is not implemented in any generality yet::
 
-            sage: f = ZZ.hom(ZZ)
+            sage: f = ZZ.hom(Zp)
             sage: f.inverse_image(ZZ.ideal(2))
             Traceback (most recent call last):
             ...

ZZ.hom(ZZ) is an IdentityMorphism now which does not have an inverse_image() method. We went from NotImplementedError to AttributeError it seems.

So this really is a subtle backwards incompatible change. Hopefully nobody was just catching the NotImplementedError. I don't think there is any (reasonable) way to put a deprecation and/or get around that.

This test is also not valid because Zp is a class, not a parent.

Actually, that brings up another question, not just that you need to handle old pickles, but do we want to have a small deprecation class for RingHomomorphism_coercion? This is an implementation detail, but it is something publicly documented. So someone might have code that uses this class and that code would just break upon upgrade.

[xander.faber]

Apropos of the _parent question, there was an example of such behavior in the doctest of the method I was fixing:

sage: Hom(ZZ,Zmod(1)).an_element().parent()
Set of Homomorphisms from Integer Ring to Ring of integers modulo 1
sage: Hom(ZZ,Zmod(1)).an_element()._parent
...
AttributeError: 'sage.rings.morphism.RingHomomorphism_im_gens' object has no attribute '_parent'

[tscrim]

No, you cannot get _parent in a Sage session as it is a C(ython) variable. However, within Cython code this should not be a problem.

[roed]

Replying to tscrim:

Replying to saraedum:

Replying to tscrim:

Replying to saraedum:

David Roe and I looked at your comments.

Replying to tscrim:

A strong -1 on calling the category (first): this is relatively expensive, not likely able to be implemented by the MorphismMethods of a category if you have to do formal compositions, and is meant to be overwritten. Really, if you want composite maps, then you should overwrite the _composition_ hook.

We don't understand what you are trying to say here. Could you rephrase your concern please.

You added two blocks for formal composite functions: [snip] What I am saying is this is really (relatively) expensive for something that is almost certainly going to be implemented. If your category has morphisms such that you cannot effectively simplify a composition f \circ g to a single map h, then I don't see how you would have enough (generic) information to check that the composition is injective/surjective. So I doubt this would ever get used.

I do not understand how this relates to _composition_. The point here is simply that in the category of Rings, a morphism that has a field as its domain, knows that it is injective. However, we can not so easily turn the composition into one morphism.

So you're doing implementing that at the category level? I feel like for something like this you should have a generic field morphism class that knows outright that it is injective and implements a custom _composition_ that returns the same class because it is completely determined by the image of 1. So the composition would amount to chasing through each map what happens to 1. If you are using some more specialized morphisms and need the generic composition, then let the generic code of the composition do its thing. I don't see the point of slowing everything else down for a special case that is more effectively dealt with by a concrete class.

Morphisms from fields aren't completely determined by where they send 1 (they have to send 1 to 1). For example, complex conjugation is a morphism of fields. Moreover, fields can embed into lots of kinds of rings that aren't fields, and you don't want to require a concrete class for each of these situations. One of the reasons for the existence of the category framework is to enable writing generic code like this.

I agree. What were the elements without _parent?

My point is that never happens. All elements must inherit from Element (or you are basically reimplementing Element with less features).

Yep, this should get fixed.

I don't understand why this needs to be a more generic class: [snip]

Because some lift map is now not inheriting from RingMap anymore.

That smells a little like you have encountered a bug and something really should be returning a RingMap_lift. Either that or we should also remove RingMap_lift with some more generic class for lifts (if that is what we decide to do, we can do that here or on a followup).

Yes, we should remove RingMap_lift. I don't remember the details but changing this type was not a hack to get things to compile.

I would call it a temporary fix until we remove RingMap_list. However, will that be a followup or done here? There is good reason to do it here as we are already touching a lot of places and is similar in spirit, but it would be a definite expansion of scope. I leave it up to you.

I think we should create a followup ticket to remove RingMap_lift, since I think it will take a nontrivial amount of work.

I am a little wary of removing RingHomomorphism.__nonzero__ as that specialized code does do a check that could be faster and might be a little better behaved because no equality checking is involved. I just want to make sure you really checked this doesn't introduce a speed regression and works well for inexact rings (which may not be fully doctested).

We think that you will never get to the == check in the new implementation in the case of ring homomorphism (unless the codomain is trivial). We checked a few cases and there was no speed regression.

That does not seem possible. If the ring homset has no zero, then is relatively expensive because of the throwing and catching of an error and is something done every time. If there is a zero map, then it should only affect the first time it is called, but then it has to do a comparison because of the default Element.__nonzero__. What were the cases you tested?

Sorry, I had misread your initial comment. There is probably a speed regression but when would this possibly be relevant (i.e., when do you create lots of rings and check that morphisms between them are nonzero? without knowing that you are talking about rings.)

sage: f = ZZ.hom(Zp(3))
sage: %timeit bool(f)
1000000 loops, best of 3: 212 ns per loop # before
10000 loops, best of 3: 25.5 µs per loop # after

I don't have a use case for it, but I do not see the point in deliberately introducing a regression by removing code that is (was?) working perfectly well. Someone might have a use case for it; I know people create lots of polynomial rings over finite fields as the field size varies, and they might be doing a check for a nonzero map in that code.

If you're creating polynomial rings over finite fields, then the morphism can never be zero so you won't be calling this code. The only time you can have a zero morphism is if the codomain is the zero ring. It seems unlikely that you're going to be testing this enough that the difference between 200ns and 20µs

The benefit is that we are now able to use a general framework for morphisms between rings that don't inherit from RingHomomorphism, for example the coercion map from ZZ to QQ. In my opinion, this is valuable enough to merit the speed regression in this case.

Finally, Julian and I talked about it and we don't see an easy way to add the custom code back in. Since __nonzero__ is a double underscore method, we can't use cached_method there, and you can't productively use cached_method on zero since it won't cache the fact that it raised an error. Moreover, if we try to implement this in the category, then in order to hook code into __nonzero__ we have to make it slower for all other elements, which doesn't seem reasonable.

Could you explain why you needed to change this doctest:

@@ -861,7 +808,7 @@ cdef class RingHomomorphism(RingMap):
 
         This is not implemented in any generality yet::
 
-            sage: f = ZZ.hom(ZZ)
+            sage: f = ZZ.hom(Zp)
             sage: f.inverse_image(ZZ.ideal(2))
             Traceback (most recent call last):
             ...

ZZ.hom(ZZ) is an IdentityMorphism now which does not have an inverse_image() method. We went from NotImplementedError to AttributeError it seems.

So this really is a subtle backwards incompatible change. Hopefully nobody was just catching the NotImplementedError. I don't think there is any (reasonable) way to put a deprecation and/or get around that.

I don't see a way to deprecate it either, but I'm not too worried about it.

This test is also not valid because Zp is a class, not a parent.

Agreed; I think Julian is fixing this now.

Actually, that brings up another question, not just that you need to handle old pickles, but do we want to have a small deprecation class for RingHomomorphism_coercion? This is an implementation detail, but it is something publicly documented. So someone might have code that uses this class and that code would just break upon upgrade.

Yeah, it seems reasonable to add something like

from sage.misc.superseded import deprecation
class RingHomomorphism_coercion(RingHomomorphism):
    def __init__(self, *args, **kwds):
        deprecation(23204, "Don't use this")
        RingHomomorphism.__init__(self, *args, **kwds)

[roed]

I am a little wary of removing RingHomomorphism.__nonzero__ as that specialized code does do a check that could be faster and might be a little better behaved because no equality checking is involved. I just want to make sure you really checked this doesn't introduce a speed regression and works well for inexact rings (which may not be fully doctested).

We think that you will never get to the == check in the new implementation in the case of ring homomorphism (unless the codomain is trivial). We checked a few cases and there was no speed regression.

That does not seem possible. If the ring homset has no zero, then is relatively expensive because of the throwing and catching of an error and is something done every time. If there is a zero map, then it should only affect the first time it is called, but then it has to do a comparison because of the default Element.__nonzero__. What were the cases you tested?

Sorry, I had misread your initial comment. There is probably a speed regression but when would this possibly be relevant (i.e., when do you create lots of rings and check that morphisms between them are nonzero? without knowing that you are talking about rings.)

sage: f = ZZ.hom(Zp(3))
sage: %timeit bool(f)
1000000 loops, best of 3: 212 ns per loop # before
10000 loops, best of 3: 25.5 µs per loop # after

I don't have a use case for it, but I do not see the point in deliberately introducing a regression by removing code that is (was?) working perfectly well. Someone might have a use case for it; I know people create lots of polynomial rings over finite fields as the field size varies, and they might be doing a check for a nonzero map in that code.

If you're creating polynomial rings over finite fields, then the morphism can never be zero so you won't be calling this code. The only time you can have a zero morphism is if the codomain is the zero ring. It seems unlikely that you're going to be testing this enough that the difference between 200ns and 20µs

The benefit is that we are now able to use a general framework for morphisms between rings that don't inherit from RingHomomorphism, for example the coercion map from ZZ to QQ. In my opinion, this is valuable enough to merit the speed regression in this case.

Finally, Julian and I talked about it and we don't see an easy way to add the custom code back in. Since __nonzero__ is a double underscore method, we can't use cached_method there, and you can't productively use cached_method on zero since it won't cache the fact that it raised an error. Moreover, if we try to implement this in the category, then in order to hook code into __nonzero__ we have to make it slower for all other elements, which doesn't seem reasonable.

Okay, we came up with some situations where the speed might matter. For example, if you call coerce_map_from it returns either a morphism or None. It's possible that people then use the result in a boolean context to determine whether it's None.

We'll try to improve the speed.

[tscrim]

Replying to roed:

Replying to tscrim:

Replying to saraedum:

Replying to tscrim:

Replying to saraedum:

David Roe and I looked at your comments.

Replying to tscrim:

A strong -1 on calling the category (first): this is relatively expensive, not likely able to be implemented by the MorphismMethods of a category if you have to do formal compositions, and is meant to be overwritten. Really, if you want composite maps, then you should overwrite the _composition_ hook.

We don't understand what you are trying to say here. Could you rephrase your concern please.

You added two blocks for formal composite functions: [snip] What I am saying is this is really (relatively) expensive for something that is almost certainly going to be implemented. If your category has morphisms such that you cannot effectively simplify a composition f \circ g to a single map h, then I don't see how you would have enough (generic) information to check that the composition is injective/surjective. So I doubt this would ever get used.

I do not understand how this relates to _composition_. The point here is simply that in the category of Rings, a morphism that has a field as its domain, knows that it is injective. However, we can not so easily turn the composition into one morphism.

So you're doing implementing that at the category level? I feel like for something like this you should have a generic field morphism class that knows outright that it is injective and implements a custom _composition_ that returns the same class because it is completely determined by the image of 1. So the composition would amount to chasing through each map what happens to 1. If you are using some more specialized morphisms and need the generic composition, then let the generic code of the composition do its thing. I don't see the point of slowing everything else down for a special case that is more effectively dealt with by a concrete class.

Morphisms from fields aren't completely determined by where they send 1 (they have to send 1 to 1). For example, complex conjugation is a morphism of fields. Moreover, fields can embed into lots of kinds of rings that aren't fields, and you don't want to require a concrete class for each of these situations. One of the reasons for the existence of the category framework is to enable writing generic code like this.

Ah, right. However, you are not doing general code, you are doing a special case, which IMO is a relatively small set of the cases, in a concrete class. Because of that, it should be a separate class if you really need that. To me, the category framework is primarily for general code that is *not* implemented in a concrete class, but can be used as a fallback, and often times, it is slow. I strongly believe that there is no practical advantage in any situation to having to do a Python call up to through the category framework instead of just letting the composite map do its thing. Moreover, I believe there will be a penalty for this extra (useless) call up through the category in general; there is typically not enough structure to say anything.

I think we should create a followup ticket to remove RingMap_lift, since I think it will take a nontrivial amount of work.

Please create it and cc me on it; I will be happy to review it.

I am a little wary of removing RingHomomorphism.__nonzero__ as that specialized code does do a check that could be faster and might be a little better behaved because no equality checking is involved. I just want to make sure you really checked this doesn't introduce a speed regression and works well for inexact rings (which may not be fully doctested).

We think that you will never get to the == check in the new implementation in the case of ring homomorphism (unless the codomain is trivial). We checked a few cases and there was no speed regression.

That does not seem possible. If the ring homset has no zero, then is relatively expensive because of the throwing and catching of an error and is something done every time. If there is a zero map, then it should only affect the first time it is called, but then it has to do a comparison because of the default Element.__nonzero__. What were the cases you tested?

Sorry, I had misread your initial comment. There is probably a speed regression but when would this possibly be relevant (i.e., when do you create lots of rings and check that morphisms between them are nonzero? without knowing that you are talking about rings.)

sage: f = ZZ.hom(Zp(3))
sage: %timeit bool(f)
1000000 loops, best of 3: 212 ns per loop # before
10000 loops, best of 3: 25.5 µs per loop # after

I don't have a use case for it, but I do not see the point in deliberately introducing a regression by removing code that is (was?) working perfectly well. Someone might have a use case for it; I know people create lots of polynomial rings over finite fields as the field size varies, and they might be doing a check for a nonzero map in that code.

If you're creating polynomial rings over finite fields, then the morphism can never be zero so you won't be calling this code. The only time you can have a zero morphism is if the codomain is the zero ring. It seems unlikely that you're going to be testing this enough that the difference between 200ns and 20µs

The benefit is that we are now able to use a general framework for morphisms between rings that don't inherit from RingHomomorphism, for example the coercion map from ZZ to QQ. In my opinion, this is valuable enough to merit the speed regression in this case.

This is not an argument for removing __nonzero__; only for having the coercion maps not be a subclass of RingHomomorphism. If they don't inherit from RingHomomorphism, then fine, they do not benefit. However, what about all of those classes that do? You are penalizing them for no reason. Also, the category framework is in some sense a (sequence of) abstract (Python) base classes; if you need to have your own hierarchy of (C/Python) classes, then there is no need to try to force things up to the category level.

Finally, Julian and I talked about it and we don't see an easy way to add the custom code back in. Since __nonzero__ is a double underscore method, we can't use cached_method there, and you can't productively use cached_method on zero since it won't cache the fact that it raised an error. Moreover, if we try to implement this in the category, then in order to hook code into __nonzero__ we have to make it slower for all other elements, which doesn't seem reasonable.

Huh? You cannot use a @cached_method only because it is a Cython class, which needs to handle the special double underscore methods differently. For Python classes, you can use @cached_method on, e.g., __nonzero__. However, this is unrelated because there is/was no caching going on.

Lastly, just as a word of caution, moving things up into a category forces them to be Python code, which is inherently slower than Cython code. Also, what is your end-game with the ring morphism code? Do you have a plan for what you want it to be like when all the changes are done?

[git]

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

​7ba221d

Deprecate is_RingHomomorphism

[saraedum]

New commits:

​7ba221d

Deprecate is_RingHomomorphism

---

New commits:

​7ba221d

Deprecate is_RingHomomorphism

[git]

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

​71db264

Remove _parent,parent() distinction

[saraedum]

New commits:

​71db264

Remove _parent,parent() distinction

---

New commits:

​71db264

Remove _parent,parent() distinction

[git]

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

​dbed916	Put RingHomomorphism_coercion back in
​1b5e6bb	fix typo

[saraedum]

New commits:

​dbed916	Put RingHomomorphism_coercion back in
​1b5e6bb	fix typo

---

New commits:

​dbed916	Put RingHomomorphism_coercion back in
​1b5e6bb	fix typo

[git]

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

​20ec906

Speed up __nonzero__

[saraedum]

New commits:

​20ec906

Speed up __nonzero__

---

New commits:

​20ec906

Speed up __nonzero__

[saraedum]

Replying to tscrim:

Replying to roed:

Replying to tscrim:

Replying to saraedum:

Replying to tscrim:

Replying to saraedum:

David Roe and I looked at your comments.

Replying to tscrim:

A strong -1 on calling the category (first): this is relatively expensive, not likely able to be implemented by the MorphismMethods of a category if you have to do formal compositions, and is meant to be overwritten. Really, if you want composite maps, then you should overwrite the _composition_ hook.

We don't understand what you are trying to say here. Could you rephrase your concern please.

You added two blocks for formal composite functions: [snip] What I am saying is this is really (relatively) expensive for something that is almost certainly going to be implemented. If your category has morphisms such that you cannot effectively simplify a composition f \circ g to a single map h, then I don't see how you would have enough (generic) information to check that the composition is injective/surjective. So I doubt this would ever get used.

I do not understand how this relates to _composition_. The point here is simply that in the category of Rings, a morphism that has a field as its domain, knows that it is injective. However, we can not so easily turn the composition into one morphism.

So you're doing implementing that at the category level? I feel like for something like this you should have a generic field morphism class that knows outright that it is injective and implements a custom _composition_ that returns the same class because it is completely determined by the image of 1. So the composition would amount to chasing through each map what happens to 1. If you are using some more specialized morphisms and need the generic composition, then let the generic code of the composition do its thing. I don't see the point of slowing everything else down for a special case that is more effectively dealt with by a concrete class.

Morphisms from fields aren't completely determined by where they send 1 (they have to send 1 to 1). For example, complex conjugation is a morphism of fields. Moreover, fields can embed into lots of kinds of rings that aren't fields, and you don't want to require a concrete class for each of these situations. One of the reasons for the existence of the category framework is to enable writing generic code like this.

Ah, right. However, you are not doing general code, you are doing a special case, which IMO is a relatively small set of the cases, in a concrete class. Because of that, it should be a separate class if you really need that. To me, the category framework is primarily for general code that is *not* implemented in a concrete class, but can be used as a fallback, and often times, it is slow. I strongly believe that there is no practical advantage in any situation to having to do a Python call up to through the category framework instead of just letting the composite map do its thing. Moreover, I believe there will be a penalty for this extra (useless) call up through the category in general; there is typically not enough structure to say anything.

We are removing a wrapper class. This should make things like __call__ faster which is probably much more important. Also, in this case there is enough structure in the category of rings to say interesting things.

I think we should create a followup ticket to remove RingMap_lift, since I think it will take a nontrivial amount of work.

Please create it and cc me on it; I will be happy to review it.

#23500.

I am a little wary of removing RingHomomorphism.__nonzero__ as that specialized code does do a check that could be faster and might be a little better behaved because no equality checking is involved. I just want to make sure you really checked this doesn't introduce a speed regression and works well for inexact rings (which may not be fully doctested).

We think that you will never get to the == check in the new implementation in the case of ring homomorphism (unless the codomain is trivial). We checked a few cases and there was no speed regression.

That does not seem possible. If the ring homset has no zero, then is relatively expensive because of the throwing and catching of an error and is something done every time. If there is a zero map, then it should only affect the first time it is called, but then it has to do a comparison because of the default Element.__nonzero__. What were the cases you tested?

Sorry, I had misread your initial comment. There is probably a speed regression but when would this possibly be relevant (i.e., when do you create lots of rings and check that morphisms between them are nonzero? without knowing that you are talking about rings.)

sage: f = ZZ.hom(Zp(3))
sage: %timeit bool(f)
1000000 loops, best of 3: 212 ns per loop # before
10000 loops, best of 3: 25.5 µs per loop # after

I don't have a use case for it, but I do not see the point in deliberately introducing a regression by removing code that is (was?) working perfectly well. Someone might have a use case for it; I know people create lots of polynomial rings over finite fields as the field size varies, and they might be doing a check for a nonzero map in that code.

If you're creating polynomial rings over finite fields, then the morphism can never be zero so you won't be calling this code. The only time you can have a zero morphism is if the codomain is the zero ring. It seems unlikely that you're going to be testing this enough that the difference between 200ns and 20µs

The benefit is that we are now able to use a general framework for morphisms between rings that don't inherit from RingHomomorphism, for example the coercion map from ZZ to QQ. In my opinion, this is valuable enough to merit the speed regression in this case.

This is not an argument for removing __nonzero__; only for having the coercion maps not be a subclass of RingHomomorphism. If they don't inherit from RingHomomorphism, then fine, they do not benefit. However, what about all of those classes that do? You are penalizing them for no reason. Also, the category framework is in some sense a (sequence of) abstract (Python) base classes; if you need to have your own hierarchy of (C/Python) classes, then there is no need to try to force things up to the category level.

We have put it back in.

Finally, Julian and I talked about it and we don't see an easy way to add the custom code back in. Since __nonzero__ is a double underscore method, we can't use cached_method there, and you can't productively use cached_method on zero since it won't cache the fact that it raised an error. Moreover, if we try to implement this in the category, then in order to hook code into __nonzero__ we have to make it slower for all other elements, which doesn't seem reasonable.

Huh? You cannot use a @cached_method only because it is a Cython class, which needs to handle the special double underscore methods differently. For Python classes, you can use @cached_method on, e.g., __nonzero__. However, this is unrelated because there is/was no caching going on.

Ok. We were just discussing ways of making it faster. We have implemented one.

Lastly, just as a word of caution, moving things up into a category forces them to be Python code, which is inherently slower than Cython code. Also, what is your end-game with the ring morphism code? Do you have a plan for what you want it to be like when all the changes are done?

The idea is not to require morphisms of rings to inherit form RingHomomorphism. They should mostly inherit from whatever makes their implementation easiest. So, we certainly do not want to get rid of classes such as RingHomomorphism_im_gens.

[git]

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

​99669ca	remove tabs
​4eb236d	fix doctests

[tscrim]

Replying to saraedum:

Replying to tscrim:

Replying to roed:

Morphisms from fields aren't completely determined by where they send 1 (they have to send 1 to 1). For example, complex conjugation is a morphism of fields. Moreover, fields can embed into lots of kinds of rings that aren't fields, and you don't want to require a concrete class for each of these situations. One of the reasons for the existence of the category framework is to enable writing generic code like this.

Ah, right. However, you are not doing general code, you are doing a special case, which IMO is a relatively small set of the cases, in a concrete class. Because of that, it should be a separate class if you really need that. To me, the category framework is primarily for general code that is *not* implemented in a concrete class, but can be used as a fallback, and often times, it is slow. I strongly believe that there is no practical advantage in any situation to having to do a Python call up to through the category framework instead of just letting the composite map do its thing. Moreover, I believe there will be a penalty for this extra (useless) call up through the category in general; there is typically not enough structure to say anything.

We are removing a wrapper class. This should make things like __call__ faster which is probably much more important. Also, in this case there is enough structure in the category of rings to say interesting things.

Fields, yes; rings, I am not so sure. I work in categories with even less structure (Lie algebras), but still could see formal composite morphisms. Also, the wrapper class only did two *cython* function calls, which should be faster that the extra Python call(s) + MRO search, especially when there is no such method. I still strongly believe very few categories will ever be able to implement a generic is_injective/is_surjective to make this worthwhile.

Finally, Julian and I talked about it and we don't see an easy way to add the custom code back in. Since __nonzero__ is a double underscore method, we can't use cached_method there, and you can't productively use cached_method on zero since it won't cache the fact that it raised an error. Moreover, if we try to implement this in the category, then in order to hook code into __nonzero__ we have to make it slower for all other elements, which doesn't seem reasonable.

Huh? You cannot use a @cached_method only because it is a Cython class, which needs to handle the special double underscore methods differently. For Python classes, you can use @cached_method on, e.g., __nonzero__. However, this is unrelated because there is/was no caching going on.

Ok. We were just discussing ways of making it faster. We have implemented one.

Did you try just implementing __nonzero__ for the category? I forget if this works with extension classes and special methods. There doesn't seem to be an implementation of __nonzero__ otherwise in the MRO.

Lastly, just as a word of caution, moving things up into a category forces them to be Python code, which is inherently slower than Cython code. Also, what is your end-game with the ring morphism code? Do you have a plan for what you want it to be like when all the changes are done?

The idea is not to require morphisms of rings to inherit form RingHomomorphism. They should mostly inherit from whatever makes their implementation easiest. So, we certainly do not want to get rid of classes such as RingHomomorphism_im_gens.

There are some good reasons to have these concrete classes, with some stemming from a general lack of structure of ring morphisms. In some ways, by removing RingHomomorphism_coercion, we have made a problem for ourselves because coercion maps become the only maps not to inherit from RingHomomorphism. We can easily make it a requirement that morphisms inherit from RingHomomorphism as really nothing is (currently) implemented across multiple categories. I guess my question is do you see some larger hierarchy of classes that will run parallel to the category hierarchy?

[roed]

Replying to tscrim:

Replying to saraedum:

Replying to tscrim:

Replying to roed:

Morphisms from fields aren't completely determined by where they send 1 (they have to send 1 to 1). For example, complex conjugation is a morphism of fields. Moreover, fields can embed into lots of kinds of rings that aren't fields, and you don't want to require a concrete class for each of these situations. One of the reasons for the existence of the category framework is to enable writing generic code like this.

Ah, right. However, you are not doing general code, you are doing a special case, which IMO is a relatively small set of the cases, in a concrete class. Because of that, it should be a separate class if you really need that. To me, the category framework is primarily for general code that is *not* implemented in a concrete class, but can be used as a fallback, and often times, it is slow. I strongly believe that there is no practical advantage in any situation to having to do a Python call up to through the category framework instead of just letting the composite map do its thing. Moreover, I believe there will be a penalty for this extra (useless) call up through the category in general; there is typically not enough structure to say anything.

We are removing a wrapper class. This should make things like __call__ faster which is probably much more important. Also, in this case there is enough structure in the category of rings to say interesting things.

Fields, yes; rings, I am not so sure. I work in categories with even less structure (Lie algebras), but still could see formal composite morphisms. Also, the wrapper class only did two *cython* function calls, which should be faster that the extra Python call(s) + MRO search, especially when there is no such method.

Of course we're not going to beat two cython function calls, but I don't think that that should be the standard. There are times when speed regressions are acceptable.

I still strongly believe very few categories will ever be able to implement a generic is_injective/is_surjective to make this worthwhile.

I don't see how it matters whether there are other categories that can do so. It's possible in Rings(), and there are multiple tickets waiting on this functionality. How does removing RingHomomorphism_coercion affect other categories anyway?

Lastly, just as a word of caution, moving things up into a category forces them to be Python code, which is inherently slower than Cython code. Also, what is your end-game with the ring morphism code? Do you have a plan for what you want it to be like when all the changes are done?

The idea is not to require morphisms of rings to inherit form RingHomomorphism. They should mostly inherit from whatever makes their implementation easiest. So, we certainly do not want to get rid of classes such as RingHomomorphism_im_gens.

There are some good reasons to have these concrete classes, with some stemming from a general lack of structure of ring morphisms. In some ways, by removing RingHomomorphism_coercion, we have made a problem for ourselves because coercion maps become the only maps not to inherit from RingHomomorphism. We can easily make it a requirement that morphisms inherit from RingHomomorphism as really nothing is (currently) implemented across multiple categories. I guess my question is do you see some larger hierarchy of classes that will run parallel to the category hierarchy?

DefaultConvertMap is implemented across multiple categories. The coercion between ZZ and QQ does not inherit from RingHomomorphism. I suspect that there are others across Sage.

[git]

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

​626e793	Fix doctest
​e891a88	Replace isinstance(RingHomomorphism)

[tscrim]

Replying to roed:

Replying to tscrim:

Replying to saraedum:

Replying to tscrim:

Replying to roed:

Morphisms from fields aren't completely determined by where they send 1 (they have to send 1 to 1). For example, complex conjugation is a morphism of fields. Moreover, fields can embed into lots of kinds of rings that aren't fields, and you don't want to require a concrete class for each of these situations. One of the reasons for the existence of the category framework is to enable writing generic code like this.

Ah, right. However, you are not doing general code, you are doing a special case, which IMO is a relatively small set of the cases, in a concrete class. Because of that, it should be a separate class if you really need that. To me, the category framework is primarily for general code that is *not* implemented in a concrete class, but can be used as a fallback, and often times, it is slow. I strongly believe that there is no practical advantage in any situation to having to do a Python call up to through the category framework instead of just letting the composite map do its thing. Moreover, I believe there will be a penalty for this extra (useless) call up through the category in general; there is typically not enough structure to say anything.

We are removing a wrapper class. This should make things like __call__ faster which is probably much more important. Also, in this case there is enough structure in the category of rings to say interesting things.

Fields, yes; rings, I am not so sure. I work in categories with even less structure (Lie algebras), but still could see formal composite morphisms. Also, the wrapper class only did two *cython* function calls, which should be faster that the extra Python call(s) + MRO search, especially when there is no such method.

Of course we're not going to beat two cython function calls, but I don't think that that should be the standard. There are times when speed regressions are acceptable.

Yes, but this is not one of those times IMO.

I still strongly believe very few categories will ever be able to implement a generic is_injective/is_surjective to make this worthwhile.

I don't see how it matters whether there are other categories that can do so. It's possible in Rings(), and there are multiple tickets waiting on this functionality. How does removing RingHomomorphism_coercion affect other categories anyway?

Removing that does not. Yet, the addition of the extra code to go up through the category framework in the FormalCompositeMap does because that is a very general construction. It is also not even possible in Rings() to implement a general is_injective and is_surjective; only the first one for Fields. I also see FormalCompositeMap as a concrete implementation of a specific type of morphism, and so it is under no obligation to do things from the category.

Lastly, just as a word of caution, moving things up into a category forces them to be Python code, which is inherently slower than Cython code. Also, what is your end-game with the ring morphism code? Do you have a plan for what you want it to be like when all the changes are done?

The idea is not to require morphisms of rings to inherit form RingHomomorphism. They should mostly inherit from whatever makes their implementation easiest. So, we certainly do not want to get rid of classes such as RingHomomorphism_im_gens.

There are some good reasons to have these concrete classes, with some stemming from a general lack of structure of ring morphisms. In some ways, by removing RingHomomorphism_coercion, we have made a problem for ourselves because coercion maps become the only maps not to inherit from RingHomomorphism. We can easily make it a requirement that morphisms inherit from RingHomomorphism as really nothing is (currently) implemented across multiple categories. I guess my question is do you see some larger hierarchy of classes that will run parallel to the category hierarchy?

DefaultConvertMap is implemented across multiple categories.

Do you mean used in multiple classes in multiple categories? However, there are no subclasses or other parallel implementations AFAIK.

The coercion between ZZ and QQ does not inherit from RingHomomorphism. I suspect that there are others across Sage.

The ZZ -> QQ probably should inherit from RingHomomorphism. I would say any others that are ring morphisms should inherit for both uniformity and to take advantage of that code. There is no cost to this unless the plan is to remove RingHomomorphism altogether as an ABC (I am not sure this is the best thing going forward, but I haven't really thought about it yet).

I do support adding extra default implementations in the category, but I do not support always forcing concrete classes to make any sort of super calls, more so as the first attempted action.

From the patchbot, there are still failing doctests and the documentation does not build.

[aly.deines]

Fixed two typos in documentation. Looks good and all tests pass.

---

New commits:

​b15febb

Fixed two doctests typos.

[tscrim]

You do not get to set a positive review just because you fixed some trivial doctest failures. Code needs to be peer-reviewed, not self-reviewed. Furthermore, that was not the only problem according to the patchbot:

sage -t --long src/sage/schemes/curves/projective_curve.py  # 3 doctests failed

and again the documentation does not build according to the patchbot.

Most importantly, there is still ongoing discussion about the code in this ticket, and you do not get to unilaterally say the code is good. You cannot set a positive review until the debate is resolved.

[was]

Replying to tscrim:

You do not get to set a positive review just because you fixed some trivial doctest failures. Code needs to be peer-reviewed, not self-reviewed. Furthermore, that was not the only problem according to the patchbot:

sage -t --long src/sage/schemes/curves/projective_curve.py  # 3 doctests failed

and again the documentation does not build according to the patchbot.

Most importantly, there is still ongoing discussion about the code in this ticket, and you do not get to unilaterally say the code is good. You cannot set a positive review until the debate is resolved.

Your tone is condescending and you’re making incorrect assumptions about the person who set the positive review having not read the code, etc.

---

New commits:

​0c3d772	fix doctests
​ea35cc4	Merge branch 'u/aly.deines/remove_ringhomomorphism_coercion' of git://trac.sagemath.org/sage into t/23204/remove_ringhomomorphism_coercion
​961b615	Merge remote-tracking branch 'origin/develop' into t/23204/remove_ringhomomorphism_coercion

[saraedum]

The tests in projective_curve pass for me. Sorry, forgot --long.

[tscrim]

Replying to was:

Replying to tscrim:

You do not get to set a positive review just because you fixed some trivial doctest failures. Code needs to be peer-reviewed, not self-reviewed. Furthermore, that was not the only problem according to the patchbot:

sage -t --long src/sage/schemes/curves/projective_curve.py  # 3 doctests failed

and again the documentation does not build according to the patchbot.

Most importantly, there is still ongoing discussion about the code in this ticket, and you do not get to unilaterally say the code is good. You cannot set a positive review until the debate is resolved.

Your tone is condescending and you’re making incorrect assumptions about the person who set the positive review having not read the code, etc.

I am sorry, I did not mean to sound condescending. However, there was no information given in that post to suggest that anyone else read the code and without consideration for the discussion did annoy me.

[saraedum]

projective_curve.py works now. Let's see what the patchbot thinks about the rest.

[git]

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

​d5affa7

The "morphism" might just be a "Map", namely a DefaultConvertMap

[was]

Replying to tscrim:

Your tone is condescending and you’re making incorrect assumptions about the person who set the positive review having not read the code, etc.

I am sorry, I did not mean to sound condescending. However, there was no information given in that post to suggest that anyone else read the code and without consideration for the discussion did annoy me.

Fair enough – electronic communication can be difficult. Everybody else involved in the discussion is in the same place (at Sage Days 87).

[tscrim]

I still strongly disagree with adding the check to up the category MRO in the generic FormalCompositeMap for a few special cases. Compare:

sage: V1 = QQ^2
sage: V2 = QQ^3
sage: phi1 = (QQ^1).hom(Matrix([[1, 1]]), V1)
sage: phi2 = V1.hom(Matrix([[1, 2, 3], [4, 5, 6]]), V2)
sage: from sage.categories.map import FormalCompositeMap
sage: c1 = FormalCompositeMap(Hom(QQ^1, V2, phi1.category_for()), phi1, phi2)
sage: %timeit c1.is_injective()
The slowest run took 3132.19 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 41.9 µs per loop
sage: %timeit for _ in range(1000): c1.is_injective()
10 loops, best of 3: 40.8 ms per loop

vs before:

sage: %timeit c1.is_injective()
The slowest run took 34968.62 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 4.88 µs per loop
sage: %timeit for _ in range(1000): c1.is_injective()
100 loops, best of 3: 4.98 ms per loop

That is a 10x slowdown for checking if injectivity/surjectivity is done at the category level, and I believe that most categories will not be able to implement either. At least the only one given so far is injectivity for Fields.

[tscrim]

Also, the doc is probably failing to build because you need to do these changes (addendum, there are 2 places, this one explicitly and a similar one):

     .. WARNING::
 
-        Comparison of FpT_Polyring_section objects is not currently 
-        implemented. See :trac: `23469`. ::
-
-        sage: fprime = loads(dumps(f))
-        sage: fprime == f
-        False   
+        Comparison of ``FpT_Polyring_section`` objects is not currently
+        implemented. See :trac:`23469`. ::
+
+            sage: fprime = loads(dumps(f))
+            sage: fprime == f
+            False
 
-        sage: fprime(1+t) == f(1+t)
-        True
+            sage: fprime(1+t) == f(1+t)
+            True

Note also the trailing whitespace and other changes to the comment.

Thinking a little bit more, it probably is better to have is_RingHomomorphism as it does something non-trivial. However, my comment was more of that in some cases, it might be possible to instead replace is_RingHomomorphism with the isinstance check. This may not be possible, but I don't know the schemes code well enough to answer this.

[saraedum]

Replying to tscrim:

I still strongly disagree with adding the check to up the category MRO in the generic FormalCompositeMap for a few special cases. Compare:

sage: V1 = QQ^2
sage: V2 = QQ^3
sage: phi1 = (QQ^1).hom(Matrix([[1, 1]]), V1)
sage: phi2 = V1.hom(Matrix([[1, 2, 3], [4, 5, 6]]), V2)
sage: from sage.categories.map import FormalCompositeMap
sage: c1 = FormalCompositeMap(Hom(QQ^1, V2, phi1.category_for()), phi1, phi2)
sage: %timeit c1.is_injective()
The slowest run took 3132.19 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 41.9 µs per loop
sage: %timeit for _ in range(1000): c1.is_injective()
10 loops, best of 3: 40.8 ms per loop

vs before:

sage: %timeit c1.is_injective()
The slowest run took 34968.62 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 4.88 µs per loop
sage: %timeit for _ in range(1000): c1.is_injective()
100 loops, best of 3: 4.98 ms per loop

That is a 10x slowdown for checking if injectivity/surjectivity is done at the category level, and I believe that most categories will not be able to implement either. At least the only one given so far is injectivity for Fields.

Feel free to try the generic code first and ask the category only if that fails/throws. Then again, when the category can answer it, it's probably faster…Anyway, I just want is_injective to work. I personally do not care about a few µs in is_injective since it currently almost always throws a NotImplementedError for me anyway.

PS: I could also imagine that some categories could quickly answer False when there are dimension reasons why a morphism can not be injective/surjective.

[tscrim]

It still will work when you remove the call up to the category in FormalCompositeMap. However, in those cases, the FormalCompositeMap should still return the answer fairly quickly unless you have a long chain to test.

[git]

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

​382e2d1

fix docbuild

[saraedum]

Let's see if this fixes the docbuild. I am running tests right now. I also created a followup to improve the performance of is_injective/is_surjective #23513.

[saraedum]

All long tests pass. Documentation builds.

[was]

Hi,

There’s a lot of commits and discussion above. However, it seems to me that the entire point of this ticket is to remove some ugly code that David Roe wrote in 2007 when he was Cythonizing my morphisms code. The relevant git blame is:

03eb48258ab src/sage/rings/morphism.pyx (William Stein     2009-04-11 16:28:50 -0700  901)         return self._lift(x)
^a4928b0cf4 src/sage/rings/morphism.py  (tornaria          2006-02-11 01:13:08 +0000  902)
1d3fa4a1bbb src/sage/rings/morphism.pyx (David Roe         2007-10-05 04:10:04 -0400  903) cdef class RingHomomorphism_coercion(RingHomomorphism):
8093d7d53d0 src/sage/rings/morphism.pyx (David Roe         2007-11-11 21:53:35 +0000  904)     def __init__(self, parent, check = True):
03eb48258ab src/sage/rings/morphism.pyx (William Stein     2009-04-11 16:28:50 -0700  905)         """
4b97d1d434c src/sage/rings/morphism.pyx (Travis Scrimshaw  2012-11-04 11:16:05 -0800  906)         Initialize ``self``.
4b97d1d434c src/sage/rings/morphism.pyx (Travis Scrimshaw  2012-11-04 11:16:05 -0800  907)
03eb48258ab src/sage/rings/morphism.pyx (William Stein     2009-04-11 16:28:50 -0700  908)         INPUT:
4b97d1d434c src/sage/rings/morphism.pyx (Travis Scrimshaw  2012-11-04 11:16:05 -0800  909)
4b97d1d434c src/sage/rings/morphism.pyx (Travis Scrimshaw  2012-11-04 11:16:05 -0800  910)         - ``parent`` -- ring homset
4b97d1d434c src/sage/rings/morphism.pyx (Travis Scrimshaw  2012-11-04 11:16:05 -0800  911)
4b97d1d434c src/sage/rings/morphism.pyx (Travis Scrimshaw  2012-11-04 11:16:05 -0800  912)         - ``check`` -- bool (default: ``True``)
03eb48258ab src/sage/rings/morphism.pyx (William Stein     2009-04-11 16:28:50 -0700  913)
03eb48258ab src/sage/rings/morphism.pyx (William Stein     2009-04-11 16:28:50 -0700  914)         EXAMPLES::
03eb48258ab src/sage/rings/morphism.pyx (William Stein     2009-04-11 16:28:50 -0700  915)
03eb48258ab src/sage/rings/morphism.pyx (William Stein     2009-04-11 16:28:50 -0700  916)             sage: f = ZZ.hom(QQ); f                    # indirect doctes
t
03eb48258ab src/sage/rings/morphism.pyx (William Stein     2009-04-11 16:28:50 -0700  917)             Ring Coercion morphism:
03eb48258ab src/sage/rings/morphism.pyx (William Stein     2009-04-11 16:28:50 -0700  918)               From: Int

They did the hard work to fix all the doctests, everything passes, and the speed regression has no real impact on other code (you can just compare to None), and there is a followup ticket for that. The modified code seems much cleaner. I’m giving this a positive review.

I’ve always thought our coercion code is too complicated/ugly, so anything that cleans it up – and this surely does – is worth it. Please start discussing ideas for optimization on #23513.

(Travis – I’m sitting here talking with the authors of this code, so maybe have more context than you.)

[tscrim]

I apologize in advance for any frustration in my response. I tried to make it as constructive as I can.

If you really think that this regression in the rest of Sage is worth the speed increase for rings (to which is only benefits certain rings and there is a check to see if the ring is in Fields, which can be expensive), then leave it as a positive review. I'm not going to

Replying to was:

There’s a lot of commits and discussion above. However, it seems to me that the entire point of this ticket is to remove some ugly code that David Roe wrote in 2007 when he was Cythonizing my morphisms code. The relevant git blame is:

[snip]

They did the hard work to fix all the doctests, everything passes,

I think this alone does not merit a positive review.

and the speed regression has no real impact on other code (you can just compare to None), and there is a followup ticket for that.

That is not true. Checking in/surjectivity for a formal composite morphism that is not in the category of rings will be slowed down by a 10x factor as demonstrated above, and it is not simply comparing something to None. However, I am not fully convinced that the new code results in a speedup:

sage: K.<x> = FunctionField(QQ)
sage: L.<y> = FunctionField(QQ)
sage: M.<z> = FunctionField(QQ)
sage: f = K.hom([y])
sage: g = L.hom([z])
sage: %timeit (g*f).is_injective()
10000 loops, best of 3: 81.3 µs per loop

vs before

sage: %timeit (g*f).is_injective()
10000 loops, best of 3: 46 µs per loop

The reason why I am unsure is because the first time I run with the new code, I get ~28.5 µs. However, this does not happen on re-trails. Do you also experience this? Actually, when I removed the caching on is_injective in Rings() (for testing purposes, which is fair since this would most likely get called repeatedly on new maps if done in a tight loop), I consistently get the faster time and

sage: %timeit phi.is_injective()
The slowest run took 16.17 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 9.33 µs per loop

vs old:

sage: %timeit phi.is_injective()
The slowest run took 6.60 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 19.8 µs per loop

So this does in fact net a ~2x speedup for this case.

However, given the check for if something is in fields, that made me think of the following situation. Suppose we implement a special morphism for ring morphisms out of Z/nZ that checks for injectivity by looking at the image of 1 and the characteristic of the codomain (at least this seems like a reasonable thing to do at some point). Then compose this with some other map that knows about its injectivity (maybe checking if some sort of representation is faithful over various characteristics). With the proposed change, it would require computing the primality of n and not take advantage of the specialized code at all, resulting in a major slowdown with no clear way around other than reimplementing FormalCompositeMap.

Well, this argument is moot if we put the check for Fields() last, but the general principle holds. Some/all of the maps are specialized, but it tries the generic code that can be very expensive (maybe getting the cardinality requires iterating over the entire (large) object, but it is free as an R-module).

The modified code seems much cleaner. I’m giving this a positive review.

The git blame above is not that relevant to the code I am referring to in FormalCompositeMorphism. That added code is the only issue that I (currently) have with this ticket.

What about my comment about undoing the deprecation on is_RingHomomorphism?

I can think of some cases where not having the FormalCompositeMap call up to the category results in errors being raised that otherwise would not. Specifically, if one of the maps in the formal composition being something that cannot say anything. So it is good to have it in there, but I strongly believe it should not be called first. Instead, it should be the fallback.

I’ve always thought our coercion code is too complicated/ugly, so anything that cleans it up – and this surely does – is worth it. Please start discussing ideas for optimization on #23513.

I agree that removing the RingHomomorphism_coercion does simplify the coercion code. However, my objection is to the code added to FormalCompositeMap, which makes things more complicated IMO. I can at least hack around it when I do need the speed. I do not work only with objects in Rings() but can formally compose morphisms. Really, there is no way around this regression other than not having these checks up through the category first.

(Travis – I’m sitting here talking with the authors of this code, so maybe have more context than you.)

I am not. Also, anybody trying to understand why this change was made latter will not have this context either. Since a few people in a room are making decisions and not giving me this context, I am going to stop reviewing and writing any code involving ring morphisms/coercions. I've spent too much time, energy, and political capital working on this.

[was]

I am going to stop reviewing and writing any code involving ring morphisms/coercions.

Bummer. Hopefully, you can focus more on the millions of other interesting things you’ve done.

I've spent too much time, energy, and political capital working on this.

For what it is worth, I greatly respect and appreciation your many contributions to Sage, and your careful feedback related to this and many other ticket. Looking forward to seeing you sometime in the future at a Sage days...

[tscrim]

Replying to was:

I've spent too much time, energy, and political capital working on this.

For what it is worth, I greatly respect and appreciation your many contributions to Sage, and your careful feedback related to this and many other ticket. Looking forward to seeing you sometime in the future at a Sage days...

It is worth quite a bit, thank you. It will be nice to see you again too at a future Sage days.

Also thank you to everyone else who worked on this ticket. I do think it is overall a step in the right direction.