Morphism.is_identity() ignores base ring endomorphisms

[defeo]

Ticket #13214 introduced the .is_identity() method. The implementation only treats parents with finite number of gens, but it assumes that the base has no non-trivial automorphisms.

This gives unexpected results:

sage: K.<z> = GF(4)
sage: phi = End(K)([z^2])
sage: R.<t> = K[]
sage: psi = End(R)(phi)
sage: psi
Ring endomorphism of Univariate Polynomial Ring in t over Finite Field in z of size 2^2
  Defn: Induced from base ring by
        Ring endomorphism of Finite Field in z of size 2^2
          Defn: z |--> z + 1
sage: psi.is_identity()
True

[tscrim]

Practically a one line fix.

[git]

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

​4714ef6	Added is_identity check for morphisms induced from a base ring morphism.
​835bbb1	Merge branch 'develop' into public/rings/morphism_base_ring_identity-15478

[mmezzarobba]

Aren't there many other cases where the current default implementation would fail? If that's doable, I would be more confortable with making it more robust or moving to some subclass where the morphism is guaranteed to be the identity if it maps the elements of gens() to themselves.

[tscrim]

I don't think that's an issue since this class is about a morphism induced from a base ring morphism. Any derived class should implement its own additional checks that reflects its structure/properties (say on the generators). Do you have a more specific example?

[mmezzarobba]

Replying to tscrim:

I don't think that's an issue since this class is about a morphism induced from a base ring morphism. Any derived class should implement its own additional checks that reflects its structure/properties (say on the generators).

To prevent any misunderstanding: I agree that your change is safe, but I have the feeling that it only hides the problem instead of fixing it at the source.

Do you have a more specific example?

Here is one, directly adapted from the docstring of SetMorphism.

sage: from sage.categories.morphism import SetMorphism
sage: f = SetMorphism(Hom(QQ, QQ, Sets()), numerator)
sage: f.is_identity()
True

[tscrim]

Replying to mmezzarobba:

To prevent any misunderstanding: I agree that your change is safe, but I have the feeling that it only hides the problem instead of fixing it at the source.

I agree that there are issues with morphisms (not) believing they are the identity because of the generic implementation in Morphism assuming objects in Sage are defined by their generating sets.

However I feel like that is out of the scope of this ticket. To me, this change is not really hiding the problem as this is essentially a wrapper class, but instead fixes one part of the problem, in that it wasn't passing data along as it should.

Here is one, directly adapted from the docstring of SetMorphism.

sage: from sage.categories.morphism import SetMorphism
sage: f = SetMorphism(Hom(QQ, QQ, Sets()), numerator)
sage: f.is_identity()
True

For that example, there isn't an induced base ring morphism, so IMO it's a separate issue from this ticket.

sage: f.__class__.__mro__
(<type 'sage.categories.morphism.SetMorphism'>,
 <type 'sage.categories.morphism.Morphism'>,
 <type 'sage.categories.map.Map'>,
 <type 'sage.structure.element.Element'>,
 <type 'sage.structure.sage_object.SageObject'>,
 <type 'object'>)

However, that is not to say it isn't a bug deserving its own ticket. As I mentioned above, the problem (for this example) is that we are not considering Q as field, but as a set. So I think the generic code should be moved to some Morphism_im_gens base class (or perhaps RingHomomorphism_im_gens?) and for a general Morphism raise a NotImplementedError, but on a separate ticket.

[mmezzarobba]

Replying to tscrim:

However, that is not to say it isn't a bug deserving its own ticket. As I mentioned above, the problem (for this example) is that we are not considering Q as field, but as a set. So I think the generic code should be moved to some Morphism_im_gens base class (or perhaps RingHomomorphism_im_gens?) and for a general Morphism raise a NotImplementedError, but on a separate ticket.

Ok, I opened #17768 for that issue.

[tscrim]

Thank you for doing the review.

[jdemeyer]

Fixing this more generally in #24281 and #24298.