Opened 4 years ago
Closed 4 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 4 years ago by
- Dependencies changed from #23184 to #23184, #23211
comment:2 Changed 4 years ago by
- Branch set to u/roed/remove_ringhomomorphism_coercion
comment:3 Changed 4 years ago by
- Branch changed from u/roed/remove_ringhomomorphism_coercion to u/saraedum/remove_ringhomomorphism_coercion
comment:4 Changed 4 years ago by
- Branch changed from u/saraedum/remove_ringhomomorphism_coercion to u/roed/remove_ringhomomorphism_coercion
comment:5 Changed 4 years ago by
- Commit set to 2043ff3a6b723c7497de9561350a3118f0c096dc
comment:6 Changed 4 years ago by
- Branch changed from u/roed/remove_ringhomomorphism_coercion to u/saraedum/remove_ringhomomorphism_coercion
comment:7 Changed 4 years ago by
- Work issues set to __nonzero__ and is_zero should work like before
comment:8 Changed 4 years ago by
- Commit changed from 2043ff3a6b723c7497de9561350a3118f0c096dc to f606020c2d3d370330bdbb44fdf3f00cb3e64cf7
comment:9 Changed 4 years ago by
- Commit changed from f606020c2d3d370330bdbb44fdf3f00cb3e64cf7 to db5cee6d098314aedd80af7f7ac8e18df32c2e20
Branch pushed to git repo; I updated commit sha1. New commits:
db5cee6 | implement zero() for ring homsets
|
comment:10 Changed 4 years ago by
- Commit changed from db5cee6d098314aedd80af7f7ac8e18df32c2e20 to a42ebcf334ac70c16dfdffe052f6968fa643f858
Branch pushed to git repo; I updated commit sha1. New commits:
a42ebcf | implement __nonzero__ when there is no zero element
|
comment:11 Changed 4 years ago by
- 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
comment:12 Changed 4 years ago by
- Keywords sd87 added
comment:13 Changed 4 years ago by
- 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 4 years ago by
- Cc xcaruso added
comment:15 Changed 4 years ago by
- Cc caruso added; xcaruso removed
comment:16 Changed 4 years ago by
- 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: ↓ 20 Changed 4 years ago by
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): 15 15 cdef CategoryObject S 16 16 17 17 cdef class RingHomomorphism(RingMap): 18 cdef RingMap _lift 19 20 cdef class RingHomomorphism_coercion(RingHomomorphism): 21 pass 18 cdef Morphism _lift 22 19 23 20 cdef class RingHomomorphism_im_gens(RingHomomorphism): 24 21 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): 373 375 sage: sage.rings.morphism.is_RingHomomorphism(2/3) 374 376 False 375 377 """ 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()) 377 382 378 383 cdef class RingMap(Morphism): 379 384 """
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): 1442 1441 S = self.codomain().change_ring(R) 1443 1442 H = Hom(T,S) 1444 1443 1445 if isinstance(R, M orphism):1444 if isinstance(R, Map): 1446 1445 if R.domain() == self.base_ring(): 1447 1446 R = self.domain().ambient_space().coordinate_ring().hom(R, T.ambient_space().coordinate_ring()) 1448 1447 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 4 years ago by
- Branch changed from u/saraedum/remove_ringhomomorphism_coercion to u/xander.faber/remove_ringhomomorphism_coercion
comment:19 Changed 4 years ago by
- Branch changed from u/xander.faber/remove_ringhomomorphism_coercion to u/saraedum/remove_ringhomomorphism_coercion
comment:20 in reply to: ↑ 17 ; follow-up: ↓ 21 Changed 4 years ago by
- 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): 15 15 cdef CategoryObject S 16 16 17 17 cdef class RingHomomorphism(RingMap): 18 cdef RingMap _lift 19 20 cdef class RingHomomorphism_coercion(RingHomomorphism): 21 pass 18 cdef Morphism _lift 22 19 23 20 cdef class RingHomomorphism_im_gens(RingHomomorphism): 24 21 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): 373 375 sage: sage.rings.morphism.is_RingHomomorphism(2/3) 374 376 False 375 377 """ 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()) 377 382 378 383 cdef class RingMap(Morphism): 379 384 """ 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 ofRingHomomorphism
). I understand why this is natural, but I don't think this should be done (here) as it is a true behavior change, theis_*
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): 1442 1441 S = self.codomain().change_ring(R) 1443 1442 H = Hom(T,S) 1444 1443 1445 if isinstance(R, M orphism):1444 if isinstance(R, Map): 1446 1445 if R.domain() == self.base_ring(): 1447 1446 R = self.domain().ambient_space().coordinate_ring().hom(R, T.ambient_space().coordinate_ring()) 1448 1447 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 andIn particular, it seems like you will now get coercion morphisms that are compositions, which seems to be different behavior than before. This could have subtle effects on code and likely have a performance regression.
David is going to have another look. There should not be a speed regression. Actually the opposite should be the case. Before the composite map was wrapped in another map, now it is directly accessible. We are going to double check that this is true.
I agree with the purpose of this ticket and would like to see the ring morphism code simplified. However, I care a lot about the speed in here because polynomials are a core part of the Sage functionality.
Sure. Let's be careful about not causing any regressions here.
New commits:
a810e3c | Fixed Element.__nonzero__ and doctests
|
96b2d87 | Fixed doctest for rings.homset.RingHomset_generic.zero
|
e11de69 | Fixed doctests for FpT_Polyring_section and FpT_Fp_section
|
83a83dd | Fixed doctests for CombinatorialObject.__bool__
|
6ddff36 | Merge branch 'develop' into t/23204/remove_ringhomomorphism_coercion
|
comment:21 in reply to: ↑ 20 ; follow-up: ↓ 22 Changed 4 years ago by
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: ↓ 24 Changed 4 years ago by
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): passWhat 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 maph
, 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 withinElement
), 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 initializeElement
. So I would probably just dotry: return self != self._parent.zero() except Exception: return True # By conventionunless you want something that returns a
TypeError
orNotImplementedError
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 removeRingMap_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 forRingMap
, it can still be passed in aMorphism
. 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.
comment:23 Changed 4 years ago by
- Commit changed from 6ddff368b759e4c627053921c66d44413390df6f to 29bad40adeff929af5ec0697f6f1a65949bf497f
Branch pushed to git repo; I updated commit sha1. New commits:
cd600d8 | fix typo
|
b2f5e24 | fix typo
|
7eccce6 | Merge branch 'develop' into t/23204/remove_ringhomomorphism_coercion
|
24173a5 | fix wording
|
29bad40 | Merge branch 'u/saraedum/remove_ringhomomorphism_coercion' of git://trac.sagemath.org/sage into t/23204/remove_ringhomomorphism_coercion
|
comment:24 in reply to: ↑ 22 ; follow-up: ↓ 28 Changed 4 years ago by
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 maph
, 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 removeRingMap_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 anIdentityMorphism
now which does not have aninverse_image()
method. We went fromNotImplementedError
toAttributeError
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 4 years ago by
- 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 4 years ago by
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 4 years ago by
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: ↓ 29 ↓ 30 Changed 4 years ago by
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 maph
, 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 of1
. So the composition would amount to chasing through each map what happens to1
. 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 reimplementingElement
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 removeRingMap_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 # afterI 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 anIdentityMorphism
now which does not have aninverse_image()
method. We went fromNotImplementedError
toAttributeError
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 4 years ago by
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 # afterI 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
and20µ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 fromZZ
toFinally, 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 usecached_method
there, and you can't productively usecached_method
onzero
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.
comment:30 in reply to: ↑ 28 ; follow-up: ↓ 40 Changed 4 years ago by
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 maph
, 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 of1
. So the composition would amount to chasing through each map what happens to1
. 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 # afterI 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
and20µ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 fromZZ
to
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 usecached_method
there, and you can't productively usecached_method
onzero
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 4 years ago by
- Commit changed from 29bad40adeff929af5ec0697f6f1a65949bf497f to 7ba221d6471bef3a74570ba8b8d5059bde3cd8dc
Branch pushed to git repo; I updated commit sha1. New commits:
7ba221d | Deprecate is_RingHomomorphism
|
comment:32 Changed 4 years ago by
- 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
comment:33 Changed 4 years ago by
- Commit changed from 7ba221d6471bef3a74570ba8b8d5059bde3cd8dc to 71db2646349fc5a893404ffa89bd62b59100b8ea
Branch pushed to git repo; I updated commit sha1. New commits:
71db264 | Remove _parent,parent() distinction
|
comment:34 Changed 4 years ago by
- 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
comment:35 Changed 4 years ago by
- 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 4 years ago by
- Commit changed from 71db2646349fc5a893404ffa89bd62b59100b8ea to 1b5e6bb08288950a6f3cfa56f7c9760d7cdb5dc4
comment:37 Changed 4 years ago by
- 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
comment:38 Changed 4 years ago by
- Commit changed from 1b5e6bb08288950a6f3cfa56f7c9760d7cdb5dc4 to 20ec906aa59cfd9fadeac72399f224dbc77877c7
Branch pushed to git repo; I updated commit sha1. New commits:
20ec906 | Speed up __nonzero__
|
comment:39 Changed 4 years ago by
- Status changed from needs_work to needs_review
- Work issues check that composite maps do not cause a speed regression deleted
comment:40 in reply to: ↑ 30 ; follow-up: ↓ 43 Changed 4 years ago by
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 maph
, 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 of1
. So the composition would amount to chasing through each map what happens to1
. 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.
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 # afterI 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
and20µ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 fromZZ
toThis is not an argument for removing
__nonzero__
; only for having the coercion maps not be a subclass ofRingHomomorphism
. If they don't inherit fromRingHomomorphism
, 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 usecached_method
there, and you can't productively usecached_method
onzero
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 4 years ago by
- Commit changed from 20ec906aa59cfd9fadeac72399f224dbc77877c7 to 4eb236d12dad79e37093b80283ab05b8c5945245
comment:42 Changed 4 years ago by
- Status changed from needs_review to needs_work
- Work issues set to failing doctests
comment:43 in reply to: ↑ 40 ; follow-up: ↓ 44 Changed 4 years ago by
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 usecached_method
there, and you can't productively usecached_method
onzero
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 asRingHomomorphism_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: ↓ 47 Changed 4 years ago by
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 asRingHomomorphism_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 fromRingHomomorphism
. We can easily make it a requirement that morphisms inherit fromRingHomomorphism
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 4 years ago by
- Commit changed from 4eb236d12dad79e37093b80283ab05b8c5945245 to e891a884f8eca9e1e7d226306cb79b9cd827ee8b
comment:46 Changed 4 years ago by
- Status changed from needs_work to needs_review
- Work issues failing doctests deleted
comment:47 in reply to: ↑ 44 Changed 4 years ago by
- 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 removingRingHomomorphism_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 asRingHomomorphism_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 fromRingHomomorphism
. We can easily make it a requirement that morphisms inherit fromRingHomomorphism
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
andRingHomomorphism
. 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 4 years ago by
- Branch changed from u/saraedum/remove_ringhomomorphism_coercion to u/aly.deines/remove_ringhomomorphism_coercion
comment:49 Changed 4 years ago by
- 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:
b15febb | Fixed two doctests typos.
|
comment:50 follow-up: ↓ 52 Changed 4 years ago by
- 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 4 years ago by
- Branch changed from u/aly.deines/remove_ringhomomorphism_coercion to u/saraedum/remove_ringhomomorphism_coercion
comment:52 in reply to: ↑ 50 ; follow-up: ↓ 55 Changed 4 years ago by
- 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 failedand again the documentation does not build according to the patchbot.
Most importantly, there is still ongoing discussion about the code in this ticket, and you do not get to unilaterally say the code is good. You cannot set a positive review until the debate is resolved.
Your tone is condescending and you’re making incorrect assumptions about the person who set the positive review having not read the code, etc.
New commits:
0c3d772 | fix doctests
|
ea35cc4 | Merge branch 'u/aly.deines/remove_ringhomomorphism_coercion' of git://trac.sagemath.org/sage into t/23204/remove_ringhomomorphism_coercion
|
961b615 | Merge remote-tracking branch 'origin/develop' into t/23204/remove_ringhomomorphism_coercion
|
comment:53 Changed 4 years ago by
The tests in projective_curve pass for me. Sorry, forgot --long.
comment:54 Changed 4 years ago by
- Work issues set to failing doctests
comment:55 in reply to: ↑ 52 ; follow-up: ↓ 58 Changed 4 years ago by
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 failedand 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 4 years ago by
- 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 4 years ago by
- Commit changed from 961b6152a6187b88d9a536aa8929762f850b8d1c to d5affa709b63178eac8a74eac90b8d61ff77d209
Branch pushed to git repo; I updated commit sha1. New commits:
d5affa7 | The "morphism" might just be a "Map", namely a DefaultConvertMap
|
comment:58 in reply to: ↑ 55 Changed 4 years ago by
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: ↓ 63 Changed 4 years ago by
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 4 years ago by
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.
comment:61 Changed 4 years ago by
- Status changed from needs_review to needs_work
comment:62 Changed 4 years ago by
- Work issues set to docbuild
comment:63 in reply to: ↑ 59 Changed 4 years ago by
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 loopvs 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 loopThat 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.
comment:64 Changed 4 years ago by
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 4 years ago by
- Commit changed from d5affa709b63178eac8a74eac90b8d61ff77d209 to 382e2d180772eb9d355aa7fbf539dcc52607a379
Branch pushed to git repo; I updated commit sha1. New commits:
382e2d1 | fix docbuild
|
comment:66 Changed 4 years ago by
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 4 years ago by
- Status changed from needs_work to needs_review
- Work issues docbuild deleted
comment:68 Changed 4 years ago by
All long tests pass. Documentation builds.
comment:69 follow-up: ↓ 71 Changed 4 years ago by
- 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 4 years ago by
- Reviewers changed from Travis Scrimshaw, Aly Deines to Travis Scrimshaw, Aly Deines, William Stein
comment:71 in reply to: ↑ 69 Changed 4 years ago by
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: ↓ 73 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
- Branch changed from u/saraedum/remove_ringhomomorphism_coercion to 382e2d180772eb9d355aa7fbf539dcc52607a379
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Fixing doctest errors