#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 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:2 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:3 Changed 6 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:4 Changed 6 years ago by
- Branch set to public/rings/morphism_base_ring_identity-15478
- Status changed from new to needs_review
comment:5 Changed 6 years ago by
- Commit set to 835bbb1277865c8d156182e749e124952699f495
comment:6 Changed 6 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 6 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 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 6 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 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 6 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 in reply to: ↑ 9 Changed 6 years ago by
- 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 perhapsRingHomomorphism_im_gens
?) and for a generalMorphism
raise aNotImplementedError
, but on a separate ticket.
Ok, I opened #17768 for that issue.
comment:12 Changed 6 years ago by
- 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 3 years ago by
- Commit 835bbb1277865c8d156182e749e124952699f495 deleted
Practically a one line fix.