Opened 6 years ago
Closed 6 years ago
#21879 closed enhancement (fixed)
Implement is_injective for ring homomorphisms
Reported by: | saraedum | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.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, GitHub, GitLab) | 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 6 years ago by
Authors: | → Julian Rüth |
---|
comment:2 Changed 6 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 6 years ago by
Description: | modified (diff) |
---|
comment:4 Changed 6 years ago by
Branch: | → u/saraedum/implement_is_injective_for_ring_homomorphisms |
---|
comment:5 Changed 6 years ago by
Commit: | → 1a5fac4f83cb9e09a81acce74e0857245b315e87 |
---|---|
Status: | new → needs_review |
comment:6 Changed 6 years ago by
Commit: | 1a5fac4f83cb9e09a81acce74e0857245b315e87 → 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 6 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 6 years ago by
Cc: | roed caruso added |
---|
comment:9 Changed 6 years ago by
Description: | modified (diff) |
---|
comment:10 Changed 6 years ago by
Commit: | 689028182d31068228562360264e86af492aadf9 → 363ece8ce62dc8cb7a510607d0f34778adc4b74f |
---|
comment:11 Changed 6 years ago by
Commit: | 363ece8ce62dc8cb7a510607d0f34778adc4b74f → 2f641e23f91a98b333ab106b69590d95a79e59e5 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
2f641e2 | Add missing doctest output
|
comment:12 Changed 6 years ago by
Dependencies: | → #21893 |
---|
New commits:
2f641e2 | Add missing doctest output
|
comment:13 Changed 6 years ago by
Reviewers: | → David Roe |
---|---|
Status: | needs_review → positive_review |
Assuming that the patchbot is happy, I am.
comment:14 Changed 6 years ago by
Dependencies: | #21893 → #21893, #21894, #21895 |
---|
comment:15 Changed 6 years ago by
comment:16 Changed 6 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 6 years ago by
vbraun: I know. I will remove one of the dependencies as soon as the other one is merged.
comment:19 Changed 6 years ago by
Yes, I can replace it with a much easier one.
comment:20 Changed 6 years ago by
Dependencies: | #21893, #21894, #21895 → #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 6 years ago by
Branch: | u/saraedum/implement_is_injective_for_ring_homomorphisms → 2f641e23f91a98b333ab106b69590d95a79e59e5 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
New commits:
fixed typo
Implement is_injective as a MorphismMethod