Opened 3 years ago

Closed 2 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) Commit: 2f641e23f91a98b333ab106b69590d95a79e59e5
Dependencies: #21893, #19820, #21895 Stopgaps:

Description (last modified by saraedum)

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 saraedum

  • Authors set to Julian Rüth

comment:2 Changed 3 years ago by saraedum

  • Description modified (diff)

comment:3 Changed 3 years ago by saraedum

  • Description modified (diff)

comment:4 Changed 3 years ago by saraedum

  • Branch set to u/saraedum/implement_is_injective_for_ring_homomorphisms

comment:5 Changed 3 years ago by saraedum

  • Commit set to 1a5fac4f83cb9e09a81acce74e0857245b315e87
  • Status changed from new to needs_review

New commits:

aa5fd5cfixed typo
1a5fac4Implement is_injective as a MorphismMethod

comment:6 Changed 3 years ago by git

  • Commit changed from 1a5fac4f83cb9e09a81acce74e0857245b315e87 to 689028182d31068228562360264e86af492aadf9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6890281Implement is_injective as a MorphismMethod

comment:7 Changed 3 years ago by saraedum

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 saraedum

  • Cc roed caruso added

comment:9 Changed 3 years ago by saraedum

  • Description modified (diff)

comment:10 Changed 3 years ago by git

  • Commit changed from 689028182d31068228562360264e86af492aadf9 to 363ece8ce62dc8cb7a510607d0f34778adc4b74f

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

e10a257Cache is_injective
363ece8Remove redundant is_subring() implementations

comment:11 Changed 3 years ago by git

  • Commit changed from 363ece8ce62dc8cb7a510607d0f34778adc4b74f to 2f641e23f91a98b333ab106b69590d95a79e59e5

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

2f641e2Add missing doctest output

comment:12 Changed 3 years ago by saraedum

  • Dependencies set to #21893

New commits:

2f641e2Add missing doctest output

comment:13 Changed 3 years ago by roed

  • 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 saraedum

  • Dependencies changed from #21893 to #21893, #21894, #21895

comment:15 Changed 3 years ago by saraedum

Actually, only one of #21894 and #21895 is a dependency. Either one makes the patchbot happy.

comment:16 Changed 3 years ago by vbraun

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 saraedum

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 roed

So, should the #21894 dependency be removed?

comment:19 Changed 3 years ago by saraedum

Yes, I can replace it with a easier one.

Last edited 3 years ago by saraedum (previous) (diff)

comment:20 Changed 3 years ago by saraedum

  • 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 2 years ago by vbraun

  • Branch changed from u/saraedum/implement_is_injective_for_ring_homomorphisms to 2f641e23f91a98b333ab106b69590d95a79e59e5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.