Changes between Initial Version and Version 1 of Ticket #23204, comment 29


Ignore:
Timestamp:
07/20/17 03:00:57 (4 years ago)
Author:
roed
Comment:

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #23204, comment 29

    initial v1  
    1 Replying to [comment:28 roed]:
    2 > Replying to [comment:24 tscrim]:
    3 > > Replying to [comment:22 saraedum]:
    4 > > > Replying to [comment:21 tscrim]:
    5 > > > > Replying to [comment:20 saraedum]:
    6 > > > > > David Roe and I looked at your comments.
    7 > > > > >
    8 > > > > > Replying to [comment:17 tscrim]:
    9 > > > > > > 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.
    10 > > > > > We don't understand what you are trying to say here. Could you rephrase your concern please.
    11 > > > >
    12 > > > > You added two blocks for formal composite functions:
    13 > > > > [snip]
    14 > > > > 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.
    15 > > > 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.
    16 > >
    17 > > 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.
    18 >
    19 > 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.
    20 
    21 > > > I agree. What were the elements without `_parent`?
    22 > >
    23 > > My point is that never happens. All elements must inherit from `Element` (or you are basically reimplementing `Element` with less features).
    24 >
    25 > Yep, this should get fixed.
    26 >
    27 > > > > > > I don't understand why this needs to be a more generic class:
    28 > > > > > > [snip]
    29 > > > > > Because some lift map is now not inheriting from `RingMap` anymore.
    30 > > > > 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).
    31 > > > 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.
    32 > >
    33 > > 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.
    34 >
    35 > I think we should create a followup ticket to remove `RingMap_lift`, since I think it will take a nontrivial amount of work.
    36 >
    37 > > > > > > 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).
     1 > > > > > > 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).
    382> > > > > 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.
    393> > > > 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?
     
    5721
    5822We'll try to improve the speed.
    59  
    60 > > > > Could you explain why you needed to change this doctest:
    61 > > > > {{{#!diff
    62 > > > > @@ -861,7 +808,7 @@ cdef class RingHomomorphism(RingMap):
    63 > > > > 
    64 > > > >          This is not implemented in any generality yet::
    65 > > > > 
    66 > > > > -            sage: f = ZZ.hom(ZZ)
    67 > > > > +            sage: f = ZZ.hom(Zp)
    68 > > > >              sage: f.inverse_image(ZZ.ideal(2))
    69 > > > >              Traceback (most recent call last):
    70 > > > >              ...
    71 > > > > }}}
    72 > > > `ZZ.hom(ZZ)` is an `IdentityMorphism` now which does not have an `inverse_image()` method. We went from `NotImplementedError` to `AttributeError` it seems.
    73 > >
    74 > > 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.
    75 >
    76 > I don't see a way to deprecate it either, but I'm not too worried about it.
    77 >
    78 > > This test is also not valid because `Zp` is a class, not a parent.
    79 >
    80 > Agreed; I think Julian is fixing this now.
    81 >
    82 > > 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.
    83 >
    84 > Yeah, it seems reasonable to add something like
    85 >
    86 > {{{
    87 > from sage.misc.superseded import deprecation
    88 > class RingHomomorphism_coercion(RingHomomorphism):
    89 >     def __init__(self, *args, **kwds):
    90 >         deprecation(23204, "Don't use this")
    91 >         RingHomomorphism.__init__(self, *args, **kwds)
    92 > }}}