Opened 2 years ago

Closed 2 years ago

#23204 closed defect (fixed)

Remove RingHomomorphism_coercion

Reported by: saraedum Owned by:
Priority: major Milestone: sage-8.0
Component: commutative algebra Keywords: sd86.5, sd87
Cc: caruso Merged in:
Authors: David Roe, Julian Rüth Reviewers: Travis Scrimshaw, Aly Deines, William Stein
Report Upstream: N/A Work issues:
Branch: 382e2d1 (Commits) Commit: 382e2d180772eb9d355aa7fbf539dcc52607a379
Dependencies: #23184, #23211 Stopgaps:

Description

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

Change History (74)

comment:1 Changed 2 years ago by roed

  • Dependencies changed from #23184 to #23184, #23211

comment:2 Changed 2 years ago by roed

  • Branch set to u/roed/remove_ringhomomorphism_coercion

comment:3 Changed 2 years ago by saraedum

  • Branch changed from u/roed/remove_ringhomomorphism_coercion to u/saraedum/remove_ringhomomorphism_coercion

comment:4 Changed 2 years ago by roed

  • Branch changed from u/saraedum/remove_ringhomomorphism_coercion to u/roed/remove_ringhomomorphism_coercion

comment:5 Changed 2 years ago by git

  • Commit set to 2043ff3a6b723c7497de9561350a3118f0c096dc

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

2043ff3Fixing doctest errors

comment:6 Changed 2 years ago by saraedum

  • Branch changed from u/roed/remove_ringhomomorphism_coercion to u/saraedum/remove_ringhomomorphism_coercion

comment:7 Changed 2 years ago by saraedum

  • Work issues set to __nonzero__ and is_zero should work like before

comment:8 Changed 2 years ago by git

  • Commit changed from 2043ff3a6b723c7497de9561350a3118f0c096dc to f606020c2d3d370330bdbb44fdf3f00cb3e64cf7

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

d7eb179Remove RingHomomorphism_coercion
dbd5b0dfix morphism printing in doctests
f606020Fixed some doctests

comment:9 Changed 2 years ago by git

  • Commit changed from f606020c2d3d370330bdbb44fdf3f00cb3e64cf7 to db5cee6d098314aedd80af7f7ac8e18df32c2e20

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

db5cee6implement zero() for ring homsets

comment:10 Changed 2 years ago by git

  • Commit changed from db5cee6d098314aedd80af7f7ac8e18df32c2e20 to a42ebcf334ac70c16dfdffe052f6968fa643f858

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

a42ebcfimplement __nonzero__ when there is no zero element

comment:11 Changed 2 years ago by saraedum

  • Status changed from new to needs_info
  • Work issues changed from __nonzero__ and is_zero should work like before to check that __nonzero__ does not cause a major speed regression

New commits:

a42ebcfimplement __nonzero__ when there is no zero element

New commits:

a42ebcfimplement __nonzero__ when there is no zero element

comment:12 Changed 2 years ago by roed

  • Keywords sd87 added

comment:13 Changed 2 years ago by saraedum

  • Status changed from needs_info to needs_review
  • Work issues check that __nonzero__ does not cause a major speed regression deleted

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.

comment:14 Changed 2 years ago by saraedum

  • Cc xcaruso added

comment:15 Changed 2 years ago by saraedum

  • Cc caruso added; xcaruso removed

comment:16 Changed 2 years ago by xander.faber

  • Status changed from needs_review to needs_work

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.

comment:17 follow-up: Changed 2 years ago by 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

    diff --git a/src/sage/rings/morphism.pxd b/src/sage/rings/morphism.pxd
    index a751a23..26fbeed 100644
    a b cdef class RingMap_lift(RingMap): 
    1515    cdef CategoryObject S
    1616
    1717cdef class RingHomomorphism(RingMap):
    18     cdef RingMap _lift
    19 
    20 cdef class RingHomomorphism_coercion(RingHomomorphism):
    21     pass
     18    cdef Morphism _lift
    2219
    2320cdef class RingHomomorphism_im_gens(RingHomomorphism):
    2421    cdef __im_gens

This is almost certainly a speed regression:

  • src/sage/rings/morphism.pyx

    diff --git a/src/sage/rings/morphism.pyx b/src/sage/rings/morphism.pyx
    index 978fe44..054aea3 100644
    a b def is_RingHomomorphism(phi): 
    373375        sage: sage.rings.morphism.is_RingHomomorphism(2/3)
    374376        False
    375377    """
    376     return isinstance(phi, RingHomomorphism)
     378    # We use the category framework to determine whether something is a ring homomorphism.
     379    from sage.categories.map import Map
     380    from sage.categories.all import Rings
     381    return isinstance(phi, Map) and phi.category_for().is_subcategory(Rings())
    377382
    378383cdef class RingMap(Morphism):
    379384    """

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

    diff --git a/src/sage/schemes/generic/morphism.py b/src/sage/schemes/generic/morphism.py
    index 777e3ba..8b7aa8c 100644
    a b class SchemeMorphism_polynomial(SchemeMorphism): 
    14421441            S = self.codomain().change_ring(R)
    14431442            H = Hom(T,S)
    14441443
    1445         if isinstance(R, Morphism):
     1444        if isinstance(R, Map):
    14461445            if R.domain() == self.base_ring():
    14471446                R = self.domain().ambient_space().coordinate_ring().hom(R, T.ambient_space().coordinate_ring())
    14481447        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.

comment:18 Changed 2 years ago by xander.faber

  • Branch changed from u/saraedum/remove_ringhomomorphism_coercion to u/xander.faber/remove_ringhomomorphism_coercion

comment:19 Changed 2 years ago by saraedum

  • Branch changed from u/xander.faber/remove_ringhomomorphism_coercion to u/saraedum/remove_ringhomomorphism_coercion

comment:20 in reply to: ↑ 17 ; follow-up: Changed 2 years ago by saraedum

  • Authors set to David Roe, Julian Rüth
  • Commit changed from a42ebcf334ac70c16dfdffe052f6968fa643f858 to 6ddff368b759e4c627053921c66d44413390df6f
  • Reviewers set to Travis Scrimshaw
  • Work issues set to drop is_RingHomomorphism, consume old pickles, check that composite maps do not cause a speed regression

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

    diff --git a/src/sage/rings/morphism.pxd b/src/sage/rings/morphism.pxd
    index a751a23..26fbeed 100644
    a b cdef class RingMap_lift(RingMap): 
    1515    cdef CategoryObject S
    1616
    1717cdef class RingHomomorphism(RingMap):
    18     cdef RingMap _lift
    19 
    20 cdef class RingHomomorphism_coercion(RingHomomorphism):
    21     pass
     18    cdef Morphism _lift
    2219
    2320cdef class RingHomomorphism_im_gens(RingHomomorphism):
    2421    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

    diff --git a/src/sage/rings/morphism.pyx b/src/sage/rings/morphism.pyx
    index 978fe44..054aea3 100644
    a b def is_RingHomomorphism(phi): 
    373375        sage: sage.rings.morphism.is_RingHomomorphism(2/3)
    374376        False
    375377    """
    376     return isinstance(phi, RingHomomorphism)
     378    # We use the category framework to determine whether something is a ring homomorphism.
     379    from sage.categories.map import Map
     380    from sage.categories.all import Rings
     381    return isinstance(phi, Map) and phi.category_for().is_subcategory(Rings())
    377382
    378383cdef class RingMap(Morphism):
    379384    """

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

    diff --git a/src/sage/schemes/generic/morphism.py b/src/sage/schemes/generic/morphism.py
    index 777e3ba..8b7aa8c 100644
    a b class SchemeMorphism_polynomial(SchemeMorphism): 
    14421441            S = self.codomain().change_ring(R)
    14431442            H = Hom(T,S)
    14441443
    1445         if isinstance(R, Morphism):
     1444        if isinstance(R, Map):
    14461445            if R.domain() == self.base_ring():
    14471446                R = self.domain().ambient_space().coordinate_ring().hom(R, T.ambient_space().coordinate_ring())
    14481447        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:

a810e3cFixed Element.__nonzero__ and doctests
96b2d87Fixed doctest for rings.homset.RingHomset_generic.zero
e11de69Fixed doctests for FpT_Polyring_section and FpT_Fp_section
83a83ddFixed doctests for CombinatorialObject.__bool__
6ddff36Merge branch 'develop' into t/23204/remove_ringhomomorphism_coercion

comment:21 in reply to: ↑ 20 ; follow-up: Changed 2 years ago by 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):
             ...

comment:22 in reply to: ↑ 21 ; follow-up: Changed 2 years ago by 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.

Last edited 2 years ago by saraedum (previous) (diff)

comment:23 Changed 2 years ago by git

  • Commit changed from 6ddff368b759e4c627053921c66d44413390df6f to 29bad40adeff929af5ec0697f6f1a65949bf497f

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

cd600d8fix typo
b2f5e24fix typo
7eccce6Merge branch 'develop' into t/23204/remove_ringhomomorphism_coercion
24173a5fix wording
29bad40Merge branch 'u/saraedum/remove_ringhomomorphism_coercion' of git://trac.sagemath.org/sage into t/23204/remove_ringhomomorphism_coercion

comment:24 in reply to: ↑ 22 ; follow-up: Changed 2 years ago by 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.

comment:25 Changed 2 years ago by saraedum

  • Work issues changed from drop is_RingHomomorphism, consume old pickles, check that composite maps do not cause a speed regression to drop is_RingHomomorphism, consume old pickles, check that composite maps do not cause a speed regression, remove _parent hasattr, fix inverse image doctest

comment:26 Changed 2 years ago by 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'

comment:27 Changed 2 years ago by 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.

comment:28 in reply to: ↑ 24 ; follow-ups: Changed 2 years ago by 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)

comment:29 in reply to: ↑ 28 Changed 2 years ago by 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.

Last edited 2 years ago by roed (previous) (diff)

comment:30 in reply to: ↑ 28 ; follow-up: Changed 2 years ago by 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?

comment:31 Changed 2 years ago by git

  • Commit changed from 29bad40adeff929af5ec0697f6f1a65949bf497f to 7ba221d6471bef3a74570ba8b8d5059bde3cd8dc

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

7ba221dDeprecate is_RingHomomorphism

comment:32 Changed 2 years ago by saraedum

  • Work issues changed from drop is_RingHomomorphism, consume old pickles, check that composite maps do not cause a speed regression, remove _parent hasattr, fix inverse image doctest to consume old pickles, check that composite maps do not cause a speed regression, remove _parent hasattr, fix inverse image doctest

New commits:

7ba221dDeprecate is_RingHomomorphism

New commits:

7ba221dDeprecate is_RingHomomorphism

comment:33 Changed 2 years ago by git

  • Commit changed from 7ba221d6471bef3a74570ba8b8d5059bde3cd8dc to 71db2646349fc5a893404ffa89bd62b59100b8ea

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

71db264Remove _parent,parent() distinction

comment:34 Changed 2 years ago by saraedum

  • Work issues changed from consume old pickles, check that composite maps do not cause a speed regression, remove _parent hasattr, fix inverse image doctest to consume old pickles, check that composite maps do not cause a speed regression, fix inverse image doctest

New commits:

71db264Remove _parent,parent() distinction

New commits:

71db264Remove _parent,parent() distinction

comment:35 Changed 2 years ago by saraedum

  • Work issues changed from consume old pickles, check that composite maps do not cause a speed regression, fix inverse image doctest to check that composite maps do not cause a speed regression, fix inverse image doctest

comment:36 Changed 2 years ago by git

  • Commit changed from 71db2646349fc5a893404ffa89bd62b59100b8ea to 1b5e6bb08288950a6f3cfa56f7c9760d7cdb5dc4

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

dbed916Put RingHomomorphism_coercion back in
1b5e6bbfix typo

comment:37 Changed 2 years ago by saraedum

  • Work issues changed from check that composite maps do not cause a speed regression, fix inverse image doctest to check that composite maps do not cause a speed regression

New commits:

dbed916Put RingHomomorphism_coercion back in
1b5e6bbfix typo

New commits:

dbed916Put RingHomomorphism_coercion back in
1b5e6bbfix typo

comment:38 Changed 2 years ago by git

  • Commit changed from 1b5e6bb08288950a6f3cfa56f7c9760d7cdb5dc4 to 20ec906aa59cfd9fadeac72399f224dbc77877c7

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

20ec906Speed up __nonzero__

comment:39 Changed 2 years ago by saraedum

  • Status changed from needs_work to needs_review
  • Work issues check that composite maps do not cause a speed regression deleted

New commits:

20ec906Speed up __nonzero__

New commits:

20ec906Speed up __nonzero__

comment:40 in reply to: ↑ 30 ; follow-up: Changed 2 years ago by 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.

comment:41 Changed 2 years ago by git

  • Commit changed from 20ec906aa59cfd9fadeac72399f224dbc77877c7 to 4eb236d12dad79e37093b80283ab05b8c5945245

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

99669caremove tabs
4eb236dfix doctests

comment:42 Changed 2 years ago by saraedum

  • Status changed from needs_review to needs_work
  • Work issues set to failing doctests

comment:43 in reply to: ↑ 40 ; follow-up: Changed 2 years ago by 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?

comment:44 in reply to: ↑ 43 ; follow-up: Changed 2 years ago by 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.

comment:45 Changed 2 years ago by git

  • Commit changed from 4eb236d12dad79e37093b80283ab05b8c5945245 to e891a884f8eca9e1e7d226306cb79b9cd827ee8b

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

626e793Fix doctest
e891a88Replace isinstance(RingHomomorphism)

comment:46 Changed 2 years ago by saraedum

  • Status changed from needs_work to needs_review
  • Work issues failing doctests deleted

comment:47 in reply to: ↑ 44 Changed 2 years ago by tscrim

  • Status changed from needs_review to needs_work

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.

comment:48 Changed 2 years ago by aly.deines

  • Branch changed from u/saraedum/remove_ringhomomorphism_coercion to u/aly.deines/remove_ringhomomorphism_coercion

comment:49 Changed 2 years ago by aly.deines

  • Commit changed from e891a884f8eca9e1e7d226306cb79b9cd827ee8b to b15febbb9c7ef9be0163d572343bef5297e3611d
  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Aly Deines
  • Status changed from needs_work to positive_review

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


New commits:

b15febbFixed two doctests typos.

comment:50 follow-up: Changed 2 years ago by tscrim

  • Status changed from positive_review to needs_work

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.

comment:51 Changed 2 years ago by saraedum

  • Branch changed from u/aly.deines/remove_ringhomomorphism_coercion to u/saraedum/remove_ringhomomorphism_coercion

comment:52 in reply to: ↑ 50 ; follow-up: Changed 2 years ago by was

  • Commit changed from b15febbb9c7ef9be0163d572343bef5297e3611d to 961b6152a6187b88d9a536aa8929762f850b8d1c

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:

0c3d772fix doctests
ea35cc4Merge branch 'u/aly.deines/remove_ringhomomorphism_coercion' of git://trac.sagemath.org/sage into t/23204/remove_ringhomomorphism_coercion
961b615Merge remote-tracking branch 'origin/develop' into t/23204/remove_ringhomomorphism_coercion

comment:53 Changed 2 years ago by saraedum

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

Last edited 2 years ago by saraedum (previous) (diff)

comment:54 Changed 2 years ago by saraedum

  • Work issues set to failing doctests

comment:55 in reply to: ↑ 52 ; follow-up: Changed 2 years ago by 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.

comment:56 Changed 2 years ago by saraedum

  • Status changed from needs_work to needs_review
  • Work issues failing doctests deleted

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

comment:57 Changed 2 years ago by git

  • Commit changed from 961b6152a6187b88d9a536aa8929762f850b8d1c to d5affa709b63178eac8a74eac90b8d61ff77d209

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

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

comment:58 in reply to: ↑ 55 Changed 2 years ago by 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).

comment:59 follow-up: Changed 2 years ago by 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.

comment:60 Changed 2 years ago by 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.

Last edited 2 years ago by tscrim (previous) (diff)

comment:61 Changed 2 years ago by tscrim

  • Status changed from needs_review to needs_work

comment:62 Changed 2 years ago by tscrim

  • Work issues set to docbuild

comment:63 in reply to: ↑ 59 Changed 2 years ago by 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.

Last edited 2 years ago by saraedum (previous) (diff)

comment:64 Changed 2 years ago by 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.

comment:65 Changed 2 years ago by git

  • Commit changed from d5affa709b63178eac8a74eac90b8d61ff77d209 to 382e2d180772eb9d355aa7fbf539dcc52607a379

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

382e2d1fix docbuild

comment:66 Changed 2 years ago by 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.

comment:67 Changed 2 years ago by saraedum

  • Status changed from needs_work to needs_review
  • Work issues docbuild deleted

comment:68 Changed 2 years ago by saraedum

All long tests pass. Documentation builds.

comment:69 follow-up: Changed 2 years ago by was

  • Status changed from needs_review to positive_review

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.)

comment:70 Changed 2 years ago by saraedum

  • Reviewers changed from Travis Scrimshaw, Aly Deines to Travis Scrimshaw, Aly Deines, William Stein

comment:71 in reply to: ↑ 69 Changed 2 years ago by 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.

comment:72 follow-up: Changed 2 years ago by 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...

comment:73 in reply to: ↑ 72 Changed 2 years ago by 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.

comment:74 Changed 2 years ago by vbraun

  • Branch changed from u/saraedum/remove_ringhomomorphism_coercion to 382e2d180772eb9d355aa7fbf539dcc52607a379
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.