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

CC: @xcaruso

Component: algebra

Keywords: morphisms rings

Author: Travis Scrimshaw

Branch: 835bbb1

Reviewer: Marc Mezzarobba

Issue created by migration from https://trac.sagemath.org/ticket/15478

[tscrim]

comment:4

Practically a one line fix.

[tscrim]

Branch: public/rings/morphism_base_ring_identity-15478

[tscrim]

Author: Travis Scrimshaw

[sagetrac-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

[sagetrac-git]

Commit: 835bbb1

[mezzarobba]

comment:6

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]

comment:7

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?

[mezzarobba]

comment:8

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]

comment:9

Replying to @mezzarobba:

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.

[mezzarobba]

comment:10

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]

Reviewer: Marc Mezzarobba

[tscrim]

comment:11

Thank you for doing the review.

[vbraun]

Changed branch from public/rings/morphism_base_ring_identity-15478 to 835bbb1

[jdemeyer]

Changed commit from 835bbb1 to none

[jdemeyer]

comment:13

Fixing this more generally in #24281 and #24298.