Opened 3 years ago
Closed 3 years ago
#21879 closed enhancement (fixed)
Implement is_injective for ring homomorphisms
Reported by:  saraedum  Owned by:  

Priority:  major  Milestone:  sage7.5 
Component:  categories  Keywords:  Rings, injectivity, is_injective, MorphismMethods, is_subring 
Cc:  roed, caruso  Merged in:  
Authors:  Julian Rüth  Reviewers:  David Roe 
Report Upstream:  N/A  Work issues:  
Branch:  2f641e2 (Commits)  Commit:  2f641e23f91a98b333ab106b69590d95a79e59e5 
Dependencies:  #21893, #19820, #21895  Stopgaps: 
Description (last modified by )
Currently, is_injective
is almost never implemented:
sage: ZZ.hom(QQ).is_injective() NotImplementedError sage: QQ.hom(QQ).is_injective() NotImplementedError
This makes the method is_subring
mostly useless since its default implementation relies on is_injective
of the "natural map":
sage: GF(3).is_subring(GF(9)) NotImplementedError
The changes introduced here, implement it as a method of the morphisms in the category of Rings where we can handle many common cases of morphisms (domain is a field, ….)
Note that is_injective
is not shown in the tab completion anymore. A problem that is fixed in #21880.
Change History (21)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
 Description modified (diff)
comment:3 Changed 3 years ago by
 Description modified (diff)
comment:4 Changed 3 years ago by
 Branch set to u/saraedum/implement_is_injective_for_ring_homomorphisms
comment:5 Changed 3 years ago by
 Commit set to 1a5fac4f83cb9e09a81acce74e0857245b315e87
 Status changed from new to needs_review
comment:6 Changed 3 years ago by
 Commit changed from 1a5fac4f83cb9e09a81acce74e0857245b315e87 to 689028182d31068228562360264e86af492aadf9
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
6890281  Implement is_injective as a MorphismMethod

comment:7 Changed 3 years ago by
Note that is_injective
is now faster even for the one case where it just returned True
before.
sage: K.<x> = FunctionField(QQ) sage: f = K.hom(x) sage: %timeit f.is_injective()
Went down from 182ns to 76ns (which is of course due to the @cached_method
.) Without it, it goes up to 13µs.
comment:8 Changed 3 years ago by
 Cc roed caruso added
comment:9 Changed 3 years ago by
 Description modified (diff)
comment:10 Changed 3 years ago by
 Commit changed from 689028182d31068228562360264e86af492aadf9 to 363ece8ce62dc8cb7a510607d0f34778adc4b74f
comment:11 Changed 3 years ago by
 Commit changed from 363ece8ce62dc8cb7a510607d0f34778adc4b74f to 2f641e23f91a98b333ab106b69590d95a79e59e5
Branch pushed to git repo; I updated commit sha1. New commits:
2f641e2  Add missing doctest output

comment:12 Changed 3 years ago by
 Dependencies set to #21893
New commits:
2f641e2  Add missing doctest output

comment:13 Changed 3 years ago by
 Reviewers set to David Roe
 Status changed from needs_review to positive_review
Assuming that the patchbot is happy, I am.
comment:14 Changed 3 years ago by
 Dependencies changed from #21893 to #21893, #21894, #21895
comment:15 Changed 3 years ago by
comment:16 Changed 3 years ago by
You should just figure out what you actually want to depend on. This ticket will not be merged until all dependencies are merged.
comment:17 Changed 3 years ago by
vbraun: I know. I will remove one of the dependencies as soon as the other one is merged.
comment:18 Changed 3 years ago by
So, should the #21894 dependency be removed?
comment:19 Changed 3 years ago by
Yes, I can replace it with a easier one.
comment:20 Changed 3 years ago by
 Dependencies changed from #21893, #21894, #21895 to #21893, #19820, #21895
We do not need the full #21894. #19820 is enough to pass the doctests. Alternatively, the revert of https://git.sagemath.org/sage.git/commit?id=86b4868e9deb41940e3bed2a3289276bd7557a52 would also be enough but I think that #19820 is a more fundamental fix to the underlying problem (broken __eq__
implementations.)
comment:21 Changed 3 years ago by
 Branch changed from u/saraedum/implement_is_injective_for_ring_homomorphisms to 2f641e23f91a98b333ab106b69590d95a79e59e5
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
fixed typo
Implement is_injective as a MorphismMethod