## #15478 closed defect (fixed)

# Morphism.is_identity() ignores base ring endomorphisms

Reported by: | Luca De Feo | Owned by: | |
---|---|---|---|

Priority: | major | Milestone: | sage-6.4 |

Component: | algebra | Keywords: | morphisms rings |

Cc: | Xavier Caruso | Merged in: | |

Authors: | Travis Scrimshaw | Reviewers: | Marc Mezzarobba |

Report Upstream: | N/A | Work issues: | |

Branch: | 835bbb1 (Commits, GitHub, GitLab) | Commit: | |

Dependencies: | Stopgaps: |

### Description

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

### Change History (13)

### comment:1 Changed 9 years ago by

Milestone: | sage-6.1 → sage-6.2 |
---|

### comment:2 Changed 9 years ago by

Milestone: | sage-6.2 → sage-6.3 |
---|

### comment:3 Changed 8 years ago by

Milestone: | sage-6.3 → sage-6.4 |
---|

### comment:4 Changed 8 years ago by

Authors: | → Travis Scrimshaw |
---|---|

Branch: | → public/rings/morphism_base_ring_identity-15478 |

Status: | new → needs_review |

### comment:5 Changed 8 years ago by

Commit: | → 835bbb1277865c8d156182e749e124952699f495 |
---|

### comment:6 Changed 8 years ago by

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.

### comment:7 follow-up: 8 Changed 8 years ago by

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?

### comment:8 follow-up: 9 Changed 8 years ago by

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

### comment:9 follow-up: 10 Changed 8 years ago by

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.

### comment:10 Changed 8 years ago by

Status: | needs_review → positive_review |
---|

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

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

### comment:12 Changed 8 years ago by

Branch: | public/rings/morphism_base_ring_identity-15478 → 835bbb1277865c8d156182e749e124952699f495 |
---|---|

Resolution: | → fixed |

Status: | positive_review → closed |

### comment:13 Changed 5 years ago by

Commit: | 835bbb1277865c8d156182e749e124952699f495 |
---|

**Note:**See TracTickets for help on using tickets.

Practically a one line fix.