sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.43k stars 479 forks source link

Identities are injective and surjective #23482

Closed saraedum closed 7 years ago

saraedum commented 7 years ago

Component: algebra

Keywords: sd87

Author: Julian Rüth, Travis Scrimshaw

Branch/Commit: 5a3e684

Reviewer: Claire Tomesch, Travis Scrimshaw, Julian Rüth

Issue created by migration from https://trac.sagemath.org/ticket/23482

saraedum commented 7 years ago

Branch: u/saraedum/identities_are_injective_and_surjective

saraedum commented 7 years ago

New commits:

b283a1aIdentity morphisms are injective and surjective
saraedum commented 7 years ago

Commit: b283a1a

saraedum commented 7 years ago

Changed keywords from none to sd87

fe72680b-762e-4373-aa0c-210520f00a1d commented 7 years ago

Reviewer: cmt

fe72680b-762e-4373-aa0c-210520f00a1d commented 7 years ago
comment:6

I checked out the ticket branch, ran ./sage -ba to rebuild all of Sage including all the Cython, and ran ./sage -tp 2 --long src/sage/categories/morphism.pyx to run the relevant doctests. However, one doctest failed, as the output below shows:

bash-3.2$ ./sage -tp 2 --long src/sage/categories/morphism.pyx
too few successful tests, not using stored timings
Running doctests with ID 2017-07-20-13-16-59-0bd01dfc.
Git branch: ticket_23482
Using --optional=mpir,python2,sage
Doctesting 1 file using 2 threads.
sage -t --long src/sage/categories/morphism.pyx
**********************************************************************
File "src/sage/categories/morphism.pyx", line 429, in sage.categories.morphism.IdentityMorphism.is_surjective
Failed example:
    ZZ.hom(ZZ).is_surjective()
Exception raised:
    Traceback (most recent call last):
      File "/Sage/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 509, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Sage/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 872, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.categories.morphism.IdentityMorphism.is_surjective[0]>", line 1, in <module>
        ZZ.hom(ZZ).is_surjective()
      File "sage/categories/map.pyx", line 1204, in sage.categories.map.Map.is_surjective (/Sage/sage/src/build/cythonized/sage/categories/map.c:9227)
        raise NotImplementedError(type(self))
    NotImplementedError: <type 'sage.rings.morphism.RingHomomorphism_coercion'>
**********************************************************************
1 item had failures:
   1 of   2 in sage.categories.morphism.IdentityMorphism.is_surjective
    [105 tests, 1 failure, 0.67 s]
----------------------------------------------------------------------
sage -t --long src/sage/categories/morphism.pyx  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 0.7 seconds
    cpu time: 0.7 seconds
    cumulative wall time: 0.7 seconds

It seems like this failure is coming from the fact that the objects used in the doctests rely on RingHomomorphism_coercion, whose removal and replacement was proposed in [ticket:23204 #23204] but which this ticket isn't listed as being dependent on.

I am not sure what the owner of this ticket would prefer to do at this point.

saraedum commented 7 years ago
comment:8

You are absolutely right. Thanks for pointing this out.

saraedum commented 7 years ago

Dependencies: #23204

fe72680b-762e-4373-aa0c-210520f00a1d commented 7 years ago
comment:9

I reran ./sage -tp 2 --long src/sage/categories/morphism.pyx and now it returned that all tests passed!

saraedum commented 7 years ago
comment:10

Great :) Please add your real name in the "Reviewer" field.

saraedum commented 7 years ago

Changed reviewer from cmt to none

tscrim commented 7 years ago
comment:12

Setting to needs work because missing reviewer name.

However, I think this should be done independently of #23204, which just means writing a little smarter doctest:

sage: Hom(QQ, QQ).identity()
Identity endomorphism of Rational Field
saraedum commented 7 years ago

Reviewer: Claire Tomesch

tscrim commented 7 years ago

Changed branch from u/saraedum/identities_are_injective_and_surjective to public/algebra/identities_bijective-23482

tscrim commented 7 years ago

Changed reviewer from Claire Tomesch to Claire Tomesch, Travis Scrimshaw

tscrim commented 7 years ago

Changed commit from b283a1a to 5a3e684

tscrim commented 7 years ago
comment:14

Thinking more about this, it really is a good idea to not depend on #23204 and do a doctest that is a little more robust. My changes should be easy to review.


New commits:

5a3e684Use morphisms that are guaranteed to be the IdentityMorphism.
tscrim commented 7 years ago

Changed dependencies from #23204 to none

saraedum commented 7 years ago

Changed reviewer from Claire Tomesch, Travis Scrimshaw to Claire Tomesch, Travis Scrimshaw, Julian Rüth

saraedum commented 7 years ago

Work Issues: waiting for the patchbot → positive review

saraedum commented 7 years ago

Changed author from Julian Rüth to Julian Rüth, Travis Scrimshaw

saraedum commented 7 years ago

Changed work issues from waiting for the patchbot → positive review to none

vbraun commented 7 years ago

Changed branch from public/algebra/identities_bijective-23482 to 5a3e684