Opened 6 years ago

Closed 5 years ago

Last modified 2 years ago

#15478 closed defect (fixed)

Morphism.is_identity() ignores base ring endomorphisms

Reported by: defeo Owned by:
Priority: major Milestone: sage-6.4
Component: algebra Keywords: morphisms rings
Cc: caruso Merged in:
Authors: Travis Scrimshaw Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: 835bbb1 (Commits) 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 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:2 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:3 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:4 Changed 5 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • Branch set to public/rings/morphism_base_ring_identity-15478
  • Status changed from new to needs_review

Practically a one line fix.

comment:5 Changed 5 years ago by git

  • Commit set to 835bbb1277865c8d156182e749e124952699f495

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

4714ef6Added is_identity check for morphisms induced from a base ring morphism.
835bbb1Merge branch 'develop' into public/rings/morphism_base_ring_identity-15478

comment:6 Changed 5 years ago by 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.

comment:7 follow-up: Changed 5 years ago by 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?

comment:8 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by 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

comment:9 in reply to: ↑ 8 ; follow-up: Changed 5 years ago by 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.

comment:10 in reply to: ↑ 9 Changed 5 years ago by mmezzarobba

  • Status changed from needs_review to 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 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.

comment:11 Changed 5 years ago by tscrim

  • Reviewers set to Marc Mezzarobba

Thank you for doing the review.

comment:12 Changed 5 years ago by vbraun

  • Branch changed from public/rings/morphism_base_ring_identity-15478 to 835bbb1277865c8d156182e749e124952699f495
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 Changed 2 years ago by jdemeyer

  • Commit 835bbb1277865c8d156182e749e124952699f495 deleted

Fixing this more generally in #24281 and #24298.

Note: See TracTickets for help on using tickets.