Opened 3 years ago

Closed 2 years ago

#23190 closed enhancement (fixed)

Improve is_injective()/is_surjective() for coercions of quotient rings

Reported by: saraedum Owned by:
Priority: minor Milestone: sage-8.1
Component: commutative algebra Keywords: sd86.5, sd87, beginner
Cc: Merged in:
Authors: Julian Rüth Reviewers: Adele Bourgeois, Maarten Derickx
Report Upstream: N/A Work issues:
Branch: 8a1e18b (Commits) Commit: 8a1e18b968a830f5d816723b0c7c3e530032ba06
Dependencies: #23485, #23484, #23483, #23482 Stopgaps:

Description (last modified by saraedum)

This should return True

sage: R.<x> = ZZ[]
sage: S.<x> = QQ[]
sage: S.quo(x^2 + 1).coerce_map_from(R.quo(x^2 + 1)).is_injective()

Generally, if R→S is injective/surjective then the quotient is.

Change History (38)

comment:1 Changed 2 years ago by saraedum

  • Description modified (diff)

comment:2 Changed 2 years ago by saraedum

  • Summary changed from Improve is_injective()/in_surjective() for coercions of quotient rings to Improve is_injective()/is_surjective() for coercions of quotient rings

comment:3 Changed 2 years ago by roed

  • Keywords sd87 added

comment:4 Changed 2 years ago by saraedum

  • Branch set to u/saraedum/improve_is_injective___is_surjective___for_coercions_of_quotient_rings

comment:5 Changed 2 years ago by saraedum

  • Branch changed from u/saraedum/improve_is_injective___is_surjective___for_coercions_of_quotient_rings to u/saraedum/23190

comment:6 Changed 2 years ago by git

  • Commit set to 64776a9e28ccdbc439dace9c43068c287ad95acd

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

64776a9Implement is_injective/is_surjective for coercions of quotient rings

comment:7 Changed 2 years ago by saraedum

  • Dependencies set to #23485, #23484, #23483, #23482

comment:8 Changed 2 years ago by saraedum

  • Status changed from new to needs_review

comment:9 Changed 2 years ago by saraedum

  • Keywords beginner added

comment:10 Changed 2 years ago by abourgeois

All doctests pass on src/sage/rings/polynomial/polynomial_quotient_ring.py However, when running the doctests on src/sage/rings, I believe it requires #23204, based on the dependencies of #23482 and #23483.

comment:11 Changed 2 years ago by abourgeois

  • Reviewers set to Adele Bourgeois

comment:12 Changed 2 years ago by saraedum

  • Authors set to Julian Rüth

comment:13 Changed 2 years ago by saraedum

  • Work issues set to failing doctest

comment:14 Changed 2 years ago by git

  • Commit changed from 64776a9e28ccdbc439dace9c43068c287ad95acd to d27262a25f9b75220f3aa84d0e1fbfadb0744331

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

d27262aFix _richcmp_

comment:15 Changed 2 years ago by saraedum

  • Work issues failing doctest deleted

New commits:

d27262aFix _richcmp_

New commits:

d27262aFix _richcmp_

comment:16 follow-up: Changed 2 years ago by chapoton

  • missing enpty line after EXAMPLES::

comment:17 in reply to: ↑ 16 Changed 2 years ago by saraedum

Replying to chapoton:

  • missing enpty line after EXAMPLES::

Is this fixed already? I could not find the EXAMPLES:: you are referring to.

comment:18 Changed 2 years ago by chapoton

indeed. But there is the same issue in richcmp as in #23185

comment:19 Changed 2 years ago by git

  • Commit changed from d27262a25f9b75220f3aa84d0e1fbfadb0744331 to 46cb2f90e1e534b51086f9b07f94854674307195

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

46cb2f9Implement _richcmp_ as discussed in 23185

comment:20 Changed 2 years ago by chapoton

  • Milestone changed from sage-8.0 to sage-8.1
  • Status changed from needs_review to needs_work

does not apply

comment:21 Changed 2 years ago by git

  • Commit changed from 46cb2f90e1e534b51086f9b07f94854674307195 to e2c1e36ad9e776a3069cdf56271f9d595637ecf0

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

e2c1e36Merge branch 'develop' into t/23190/23190

comment:22 Changed 2 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:23 Changed 2 years ago by saraedum

  • Status changed from needs_review to needs_work
  • Work issues set to failing doctest

comment:24 Changed 2 years ago by saraedum

File "src/sage/rings/polynomial/polynomial_quotient_ring.py", line 1868, in sage.rings.polynomial.polynomial_quotient_ring.PolynomialQuotientRing_coercion.is_surjective
Failed example:
    f.is_surjective()
Exception raised:
    Traceback (most recent call last):
      File "/tmp/sage-patchbot/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 509, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/tmp/sage-patchbot/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 872, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.polynomial.polynomial_quotient_ring.PolynomialQuotientRing_coercion.is_surjective[2]>", line 1, in <module>
        f.is_surjective()
      File "/tmp/sage-patchbot/sage/local/lib/python2.7/site-packages/sage/rings/polynomial/polynomial_quotient_ring.py", line 1882, in is_surjective
        constant_map_is_surjective = self.codomain().base_ring().coerce_map_from(self.domain().base_ring()).is_surjective()
      File "sage/categories/map.pyx", line 1204, in sage.categories.map.Map.is_surjective (build/cythonized/sage/categories/map.c:9316)
        raise NotImplementedError(type(self))
    NotImplementedError: <type 'sage.categories.morphism.IdentityMorphism'>

comment:25 Changed 2 years ago by git

  • Commit changed from e2c1e36ad9e776a3069cdf56271f9d595637ecf0 to 944e05e05568202eebbcbe98bdb64beab39cdbe9

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

944e05eMerge branch 'develop' into t/23190/23190

comment:26 Changed 2 years ago by saraedum

  • Status changed from needs_work to needs_review
  • Work issues failing doctest deleted

Somehow #23482 is not in develop yet, that's why the tests fail.

comment:27 Changed 2 years ago by mderickx

  • Status changed from needs_review to needs_work

I think the mathematical content of the statement if R -> S is injective then the quotient is, is no true. A counter example is for example ZZ['x'].quo(2) == GF(2)['x'] which does not inject into QQ['x'].quo(2) == 0. This example is not a pathology of 2 being in the constant field, it fails for any polynomial whose content is not a unit.

A nice sufficient condition for injectivity is that the polynomial be monic (or more generally if its leading coefficient is a unit).

There are no problems with surjectivity.

Last edited 2 years ago by mderickx (previous) (diff)

comment:28 Changed 2 years ago by saraedum

  • Status changed from needs_work to needs_review

Thanks for catching that one. You are right of course. Fixed now.

comment:29 Changed 2 years ago by git

  • Commit changed from 944e05e05568202eebbcbe98bdb64beab39cdbe9 to 19aee539f2e5ba1c65f408d9a5ee40eb59e3e0e3

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

19aee53Fix is_injective

comment:30 Changed 2 years ago by git

  • Commit changed from 19aee539f2e5ba1c65f408d9a5ee40eb59e3e0e3 to 282ea209b388f2fe76b4e93f192f74e7a749bf23

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

282ea20Do not test is_jnjective() for non-unit leading coefficient

comment:31 Changed 2 years ago by mderickx

Ok the code looks fine now. If you can rewrite the extremely long lines such as:

if self.domain().modulus().change_ring(self.codomain().base_ring()) == self.codomain().modulus() and self.domain().modulus().leading_coefficient().is_unit():

in such a way that they comfortably fit on a reasonable screen and all the doctests pass. Then this ticket gets a positive review from me.

P.s. In reviewing this ticket I also found some mathematically incorrect behaviour of quotients of univariate polynomials rings. I created a separate ticket for this: #23621

Last edited 2 years ago by mderickx (previous) (diff)

comment:32 Changed 2 years ago by git

  • Commit changed from 282ea209b388f2fe76b4e93f192f74e7a749bf23 to 8a1e18b968a830f5d816723b0c7c3e530032ba06

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

8a1e18bbreak long lines

comment:33 Changed 2 years ago by saraedum

I only found that one line that's extremely long. For the others that are somewhat long I don't think there is a good position that calls for breaking them there. (But if you insist, I can break them of course.)


New commits:

8a1e18bbreak long lines
Last edited 2 years ago by saraedum (previous) (diff)

comment:34 Changed 2 years ago by saraedum

  • Reviewers changed from Adele Bourgeois to Adele Bourgeois, Maarten Derickx

comment:35 follow-up: Changed 2 years ago by chapoton

a question to abourgeois : should we write "Adele" or "Adèle" ?

comment:36 in reply to: ↑ 35 Changed 2 years ago by abourgeois

Replying to chapoton:

a question to abourgeois : should we write "Adele" or "Adèle" ?

I usually sign my name with the accent, but I don't really have a preference in writing (some of my legal documents omit the accent).

comment:37 Changed 2 years ago by mderickx

  • Status changed from needs_review to positive_review

Ok, looks good to me.

comment:38 Changed 2 years ago by vbraun

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