coerce vs _coerce_ vs _coerce_c

videlec commented 2 years ago

Even though the _coerce_ and _coerce_c methods are only implemented in the soon to be deprecated sage.structure.parent_old.Parent it is still used in some places. The current situation prevents to build a ring that inherits from Parent and use it as a base ring for the multivariate polynomial ring.

We replace all usage of _coerce_/_coerce_c by coerce and deprecate the formers.

Depends on #33525 Depends on #33576 Depends on #33558

CC: @mezzarobba

Component: algebra

Author: Vincent Delecroix, Travis Scrimshaw

Branch/Commit: 4ed53f8

Reviewer: Frédéric Chapoton, Travis Scrimshaw, Vincent Delecroix

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

videlec commented 2 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
-Even though the `_coerce_` method is only implemented in the deprecated sage.structure.parent_old.Parent it is still used in some places. The current situation prevents to build a ring that inherits from Parent and use it as a base ring for the multivariate polynomial ring.
+Even though the `_coerce_` and `_coerce_c_` methods are only implemented in the soon to be deprecated `sage.structure.parent_old.Parent` it is still used in some places. The current situation prevents to build a ring that inherits from Parent and use it as a base ring for the multivariate polynomial ring.

 We replace all usage of `_coerce_` by `coerce` and deprecate the former.

videlec commented 2 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
-Even though the `_coerce_` and `_coerce_c_` methods are only implemented in the soon to be deprecated `sage.structure.parent_old.Parent` it is still used in some places. The current situation prevents to build a ring that inherits from Parent and use it as a base ring for the multivariate polynomial ring.
+Even though the `_coerce_` and `_coerce_c` methods are only implemented in the soon to be deprecated `sage.structure.parent_old.Parent` it is still used in some places. The current situation prevents to build a ring that inherits from Parent and use it as a base ring for the multivariate polynomial ring.

-We replace all usage of `_coerce_` by `coerce` and deprecate the former.
+We replace all usage of `_coerce_`/`_coerce_c` by `coerce` and deprecate the formers.

videlec commented 2 years ago

New commits:

cd7e895 33497: _coerce_/_coerce_c -> coerce

videlec commented 2 years ago

Commit: cd7e895

videlec commented 2 years ago

Branch: u/vdelecroix/33497

tscrim commented 2 years ago

comment:4

This is definitely a step in the right direction. So +1 on this ticket. Definitely in the documentation it would be good to use coerce(). I am a little worried in the code that it might be slower in core code (integers (mod ring), rationals, and polynomial rings specifically). This might be something we just have to swallow when we finish the transition, but I am worried this might hide slightly potential optimizations. I know there are a lot of 'if's in that, so it is not a strong argument. I would probably feel better if there were some timings. What are your thoughts on this?

videlec commented 2 years ago

comment:5

Currently there is an indirection : _coerce_/_coerce_c calls coerce. So what I wrote should make everything faster. With the branch, you can see the indirection

sage: QQ._coerce_(3)
DeprecationWarning: _coerce_ is deprecated, use coerce instead
See https://trac.sagemath.org/33497 for details.
3

tscrim commented 2 years ago

comment:6

I see. Thanks. So every parent that you changed already has an element_constructor then (and hence making that an indirection)?

videlec commented 2 years ago

comment:7

I did not modify any parent. If tests pass, then all parents that are tested have an element constructor...

tscrim commented 2 years ago

comment:8

I wasn’t be precise enough, but for the parents that you call coerce() instead of _coerce_(). Test passing is not an indication because you did these changes. It would need to be tested with the deprecation but without the aforementioned change.

I am not sure that is a good thing that there is the element constructor and using the old coercion system as it means it is half way between the two, but that is not so relevant.

videlec commented 2 years ago

comment:9

Should I add a warning when the element constructor is not implemented? ie

diff --git a/src/sage/structure/parent_old.pyx b/src/sage/structure/parent_old.pyx
index 630f0ca861..02c3423a0b 100644
--- a/src/sage/structure/parent_old.pyx
+++ b/src/sage/structure/parent_old.pyx
@@ -39,7 +39,8 @@ from cpython.bool cimport *
 cdef inline check_old_coerce(Parent p):
     if p._element_constructor is not None:
         raise RuntimeError("%s still using old coercion framework" % p)
-
+    from warnings import warn
+    warn("{} does not implement element_constructor".format(p), RuntimeWarning)

 cdef class Parent(parent.Parent):
     """

tscrim commented 2 years ago

comment:10

That is a good idea for a way to check. My guess is there are still parents that use that. However, I suspect there are parents that do not implement it since it is something that comes from using the new style Parent. So I would be a little curious as to what would prevent them from moving to the new coercion framework. There also wouldn’t be any way to reach the _coerce_c() or _coerce_impl()`, right?

The change _coerce_c() -> coerce() is not removing an indirection though.

videlec commented 2 years ago

comment:11

It seems to me that only multivariate polynomial rings are going through the old coercion framework

$ git grep -l '_coerce_c_impl('
rings/polynomial/multi_polynomial_ring_base.pxd
rings/polynomial/multi_polynomial_ring_base.pyx
structure/parent_base.pyx
structure/parent_old.pxd

tscrim commented 2 years ago

comment:12

Then perhaps we can just leave those calls to _coerce_c and fully deprecate _coerce_()?

Also, the patchbot seems to report an infinite loop in schemes/hyperelliptic_curves/monsky_washnitzer.py:

      File "/amd/compute/sagebot/sage/local/var/lib/sage/venv-python3.10.2/lib/python3.10/site-packages/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 313, in __call__
        return self.coerce(value)
      File "sage/structure/parent.pyx", line 1183, in sage.structure.parent.Parent.coerce (build/cythonized/sage/structure/parent.c:11113)
        cpdef coerce(self, x):
      File "sage/structure/parent.pyx", line 1215, in sage.structure.parent.Parent.coerce (build/cythonized/sage/structure/parent.c:11059)
        return (<map.Map>mor)._call_(x)
      File "sage/categories/morphism.pyx", line 457, in sage.categories.morphism.CallMorphism._call_ (build/cythonized/sage/categories/morphism.c:6817)
        return self._codomain(x)
      File "/amd/compute/sagebot/sage/local/var/lib/sage/venv-python3.10.2/lib/python3.10/site-packages/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 313, in __call__
        return self.coerce(value)

fchapoton commented 2 years ago

comment:13

Yes, there is something going wrong in "monsky_washnitzer", probably (at least) in the class

SpecialCubicQuotientRing

which has no element constructor. Maybe it would also help to add a method "one" to this ring ?

EDIT: adding "one" does not fix the infinite recursion

fchapoton commented 2 years ago

comment:14

I pushed to trac a rough branch (public/monsky_coercion) where I tried to convert the Monsky-Washnitser file to the current coercion system. Not yet working, but possibly close to. Maybe Travis would know better what to do.

fchapoton commented 2 years ago

comment:15

I have made #33525 for the Monsky problem.

fchapoton commented 2 years ago

comment:16

I would suggest not to touch the file schemes/hyperelliptic_curves/monsky_washnitzer.py here at all.

fchapoton commented 2 years ago

comment:17

so here is a branch just the same but with no changes in monsky

New commits:

238a482 33497: _coerce_/_coerce_c -> coerce

fchapoton commented 2 years ago

Changed branch from u/vdelecroix/33497 to public/ticket/33497

fchapoton commented 2 years ago

Changed commit from cd7e895 to 238a482

fchapoton commented 2 years ago

Dependencies: #33525

fchapoton commented 2 years ago

comment:18

it seems that the pathcbot ran the doctests without merging the dependency..

tscrim commented 2 years ago

comment:19

I think it needs to be manually merged in each ticket for testing.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 238a482 to 6cd65aa

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

`d32b914`	`tentative to convert monsky to coercion system`
`c808d47`	`monsky : modern coercion`
`5abbdf2`	`adding doc`
`c2c5114`	`various changes in monsky after reviewer's suggestions`
`cbf7db6`	`one little detail`
`6cae3da`	`fix doctests`
`70362ae`	`details in doc`
`6cd65aa`	`Merge branch 'public/ticket/33497' of https://github.com/sagemath/sagetrac-mirror into public/ticket/33497`

tscrim commented 2 years ago

comment:22

I just pushed the merge. Running the tests that failed before, I get the following deprecation warnings:

sage -t --warn-long 36.7 --random-seed=61225901820605168759510913146636968794 src/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py
**********************************************************************
File "src/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 2457, in sage.schemes.hyperelliptic_curves.monsky_washnitzer.SpecialHyperellipticQuotientElement._rmul_
Failed example:
    x._rmul_(y)
Expected:
    y*1*x
**********************************************************************
File "src/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 3103, in sage.schemes.hyperelliptic_curves.monsky_washnitzer.MonskyWashnitzerDifferential._add_
Failed example:
    x*w + w
Expected:
    (1 + x) dx/2y
**********************************************************************
File "src/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 3279, in sage.schemes.hyperelliptic_curves.monsky_washnitzer.MonskyWashnitzerDifferential.extract_pow_y
Failed example:
    (A * C.invariant_differential()).extract_pow_y(5)
Expected:
    [1, 0, 0, 0, 0]
**********************************************************************
File "src/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py", line 3489, in sage.schemes.hyperelliptic_curves.monsky_washnitzer.MonskyWashnitzerDifferential.reduce_pos_y
Failed example:
    (y^2).diff().reduce_pos_y()
Expected:
    (y^2*1, 0 dx/2y)

sage -t --warn-long 36.7 --random-seed=61225901820605168759510913146636968794 src/sage/schemes/hyperelliptic_curves/hyperelliptic_padic_field.py
**********************************************************************
File "src/sage/schemes/hyperelliptic_curves/hyperelliptic_padic_field.py", line 735, in sage.schemes.hyperelliptic_curves.hyperelliptic_padic_field.coleman_integral
Failed example:
    C.coleman_integral(x*w, P, Q)
Expected:
    O(11^6)

and one proper failure:

sage -t --warn-long 36.7 --random-seed=61225901820605168759510913146636968794 src/sage/rings/finite_rings/element_pari_ffelt.pyx
**********************************************************************
File "src/sage/rings/finite_rings/element_pari_ffelt.pyx", line 1285, in sage.rings.finite_rings.element_pari_ffelt.FiniteFieldElement_pari_ffelt._gap_init_
Failed example:
    gap.coerce(a)
Expected:
    Traceback (most recent call last):
    ...
    TypeError: order must be at most 65536
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/home/travis/sage/local/var/lib/sage/venv-python3.8/lib/python3.8/site-packages/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/travis/sage/local/var/lib/sage/venv-python3.8/lib/python3.8/site-packages/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.finite_rings.element_pari_ffelt.FiniteFieldElement_pari_ffelt._gap_init_[13]>", line 1, in <module>
        gap.coerce(a)
      File "sage/structure/parent.pyx", line 1183, in sage.structure.parent.Parent.coerce (build/cythonized/sage/structure/parent.c:11113)
        cpdef coerce(self, x):
      File "sage/structure/parent.pyx", line 1213, in sage.structure.parent.Parent.coerce (build/cythonized/sage/structure/parent.c:11039)
        raise TypeError(_LazyString("no canonical coercion from %s to %s", (parent(x), self), {}))
    TypeError: no canonical coercion from Finite Field in a of size 1009^2 to Gap

fchapoton commented 2 years ago

comment:23

any idea where these deprecations comes from ? the traceback is not very helpful:

      File "<doctest sage.schemes.hyperelliptic_curves.hyperelliptic_padic_field.coleman_integral[8]>", line 1, in <module>
        C.coleman_integral(x*w, P, Q)
      File "/home/debian/sage/local/lib/python3.9/site-packages/sage/categories/pushout.py", line 4112, in pushout
        elif S.has_coerce_map_from(Rs[-1]):
      File "/home/debian/sage/local/lib/python3.9/site-packages/sage/misc/superseded.py", line 99, in deprecation
        warning(trac_number, message, DeprecationWarning, stacklevel)

EDIT: This may hint that monsky_washnitser is not yet in perfect shape..

tscrim commented 2 years ago

comment:24

What is happening is this:

parent_old.Parent._coerce_impl() -> ParentWithBase._coerce_c_impl() -> _coerce_()

(I have also deprecated the second method locally and will push once I figure out how to fix things fully since it either calls _coerce_ or errors out.) So my guess it is coming from one of these calls:

travis@tscrim:~/sage/src/sage/combinat$ grep -R "\._coerce_impl(" ../*
../algebras/commutative_dga.py:            return self._coerce_impl(im_gens)
../categories/crystals.py:            return self._coerce_impl(on_gens)
../interfaces/r.py:        return super(R, self)._coerce_impl(x, use_special=use_special)
../interfaces/polymake.py:        return super(PolymakeAbstract, self)._coerce_impl(x, use_special=use_special)
../interfaces/interface.py:            result = self._coerce_impl(x, use_special=False)
../modular/abvar/homspace.py:            sage: J = J0(37) ; J.Hom(J)._coerce_impl(matrix(ZZ,4,[5..20]))
../modular/abvar/homspace.py:            sage: K = J0(11) * J0(11) ; J.Hom(K)._coerce_impl(matrix(ZZ,4,[5..20]))
../modular/abvar/homspace.py:            return HomsetWithBase._coerce_impl(self, x)
../rings/finite_rings/homset.py:            return self._coerce_impl(im_gens)
../rings/finite_rings/homset.py:                return self._coerce_impl(im_gens)
../rings/multi_power_series_ring.py:            sage: g1 = R._coerce_impl(f1)
../rings/polynomial/pbori/pbori.pyx:            return self._coerce_impl(other)

Reducing it down to removing the obvious non-related calls:

../algebras/commutative_dga.py:            return self._coerce_impl(im_gens)
../rings/finite_rings/homset.py:            return self._coerce_impl(im_gens)
../rings/finite_rings/homset.py:                return self._coerce_impl(im_gens)
../rings/polynomial/pbori/pbori.pyx:            return self._coerce_impl(other)

It is one of these is what is starting the process to yield the error.

However, I don't think that is the root issue. Putting a print(type(self)) in parent_old.Parent._coerce_impl yields

 <class 'sage.schemes.hyperelliptic_curves.monsky_washnitzer.SpecialHyperellipticQuotientRing_with_category'>

Indeed, that is the problem as that is still using the old coercion framework, which I tested by changing it to inherit from Parent instead. I have created #33576 for this.

tscrim commented 2 years ago

Changed dependencies from #33525 to #33525, #33576

tscrim commented 2 years ago

comment:25

With the changes on #33576, the only failure left I am getting is on rings/finite_rings/element_pari_ffelt.pyx, which I believe is essentially a trivial failure because of a different error message because of the coercion change (it is reporting the same error IMO). So I propose changing the doctest output, and then all tests should pass.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

`d08f968`	`Convert MonskyWashnitzerDifferential to use the new coercion model.`
`dbcb150`	`Modernization of special hyperelliptic quotient and other fixes.`
`276d9d2`	`partially repairing monsky`
`98d0709`	`Fixing bug with not normalizing input.`
`ea8e66a`	`Merge branch 'public/coercion/special_hyperelliptic_quotient_ring-33576' into public/ticket/33497`
`74ca9d0`	`Merge branch 'develop' into public/ticket/33497`
`25ba447`	`Merge branch 'develop' into public/coercion/special_hyperelliptic_quotient_ring-33576`
`7c47495`	`Fixing pyflakes issues in monsky_washnitzer.py.`
`10ba368`	`Merge branch 'public/coercion/special_hyperelliptic_quotient_ring-33576' into public/ticket/33497`
`251cbc3`	`Fixing last doctest failure.`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 6cd65aa to 251cbc3

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 251cbc3 to 9635e1e

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

`fc660bb`	get rid of `__nonzero__` aliases for __bool__
`82dffdf`	convert `__nonzero__` to __bool__
`969cbcd`	`even less __nonzero__`
`b16bc4a`	`manual fixes for __nonzero__`
`cf98130`	`fine tuning : full removal of __nonzero__`
`b24ef83`	`Merge branch 'u/chapoton/33558' in 9.6.B6`
`9635e1e`	`Merge branch 'u/chapoton/33558' of https://github.com/sagemath/sagetrac-mirror into public/ticket/33497`

tscrim commented 2 years ago

comment:28

Here is with fixing the last doctest and the other deprecation I mentioned. I also merged in #33558 to resolve a trivial conflict.

tscrim commented 2 years ago

Changed dependencies from #33525, #33576 to #33525, #33576, #33558

tscrim commented 2 years ago

comment:29

Morally green bot (pyflakes warnings are not introduced on this ticket).

tscrim commented 2 years ago

comment:30

Ping. Should we try to get this into the next Sage release or wait for the next beta0?

videlec commented 2 years ago

comment:31

Would be great to have it in the next release! It is a major step forward in using Parent.

tscrim commented 2 years ago

comment:32

Only my latest changes need to be reviewed as I have reviewed Frédéric's and (re-)reviewed yours (as Frédéric likely already reviewed yours). If mine are good, then it is a positive review.

tscrim commented 2 years ago

Reviewer: Frédéric Chapoton, Travis Scrimshaw

videlec commented 2 years ago

Changed reviewer from Frédéric Chapoton, Travis Scrimshaw to Frédéric Chapoton, Travis Scrimshaw, Vincent Delecroix

videlec commented 2 years ago

comment:33

Looks good to me.

videlec commented 2 years ago

Changed author from Vincent Delecroix to Vincent Delecroix, Travis Scrimshaw,

tscrim commented 2 years ago

comment:35

Thank you.

vbraun commented 2 years ago

comment:36

Merge failure on top of:

5a999eb9e1 Trac #33468: Feature for gfan

b389f97a3a Trac #33466: sage.features.lrs: Make it a JoinFeature of Lrs, LrsNash; use Executable.absolute_filename

204cbf0584 Trac #33017: LazyImport.instancecheck, subclasscheck: Return False on ImportError

ae06fa42e0 Trac #31802: Bug in plotting 3d polyhedron with rays, add option polygon='rainbow'

0553c9b0b9 Trac #30749: Characteristic polynomial of central Hyperplane arrangement returns wrong result?

2317d5d7fc Trac #30411: src/tox.ini: Add environment pyright

cf219d6eb2 Trac #33661: Don't test files created by jupyter-sphinx

770a894d6c Trac #33642: Update build/pkgs/matplotlib/install-requires.txt and distros/conda.txt

a4429624dc Trac #33638: GH Actions: Fix cygwin

0a5029cfbe Trac #33430: GH Actions: Fix homebrew-maximal, remove opensuse-15.2.1

7ee048cefb Trac #33426: add more details on conda-based installations of Sage

7acdc3d662 Trac #33246: canonical_label returns incorrect partite sets

6df7b36536 Trac #33063: notebook: update to 6.4.10 to fix deprecation warning

365d2b1957 Trac #33650: Set JUPYTER_DATA_DIR etc. during docbuild

b2ddb07d71 Trac #33635: fixing the broken linter, again

c489291a97 Trac #33632: fix indentation (E111) in pyx files in numerical/

a837fee19e Trac #33626: sageinspect.sage_getfile(x) incorrect when using an alternate suffix for extension modules

216d60bb1b Trac #33633: fix indentation (E111) in pyx files in graphs, libs, proba

39d402cb75 Trac #33629: add negative for continued fractions

4955762701 Trac #33565: Add CODE_OF_CONDUCT.md to the repo

00bc51075d Trac #33560: pytest: ignore cython files

a02eec36bf Trac #33582: clean up docstring formatting in schemes/elliptic_curves/

31995b748a Trac #33618: docbuild: typo in arguments check leads to confusing error

94a131cc20 Trac #33612: sage.matrix.matrixspace: Rename a test... function to test... (with deprecation)

823194cfff Trac #33605: cbc: Add system package information for gentoo, nix

6eb1f67d62 Trac #33593: fix W391 in groups/ and algebras/

5ec6108788 Trac #33571: Correct wrong paths in the developer's guide

b886d89536 Trac #33628: Fig bug in graphs.RandomToleranceGraph

5ee8a50e85 Trac #33616: Simplify inventory builder

0a7c8f485c Trac #33615: full cleanup of modules/fp_graded/

97533a71f9 Trac #33589: Gitpod: track remote trac branch

b8d5371aae Trac #33576: Modernize coercion for SpecialHyperellipticQuotientRing

55da3e109e Trac #33572: sage -pytest

2f104e6454 Trac #33533: Update nbconvert to 6.4.x, add optional package pyppeteer

7a5714ac65 Trac #33366: BipartiteGraph() fails to create graph from graph6 string

dfd98a710e Trac #31006: Add more typing information to manifold package

360ee4c168 Trac #30540: Add package phitigra: Graph editor that works with Jupyter

3f93cd0a9c Trac #29640: Create an AUTHORS.md file with links

d8f76fb1bd Trac #33624: doctest unnecessarily assumes SAGE_VENV/bin contains sage binary

b438a78918 Trac #33608: Building documentation is broken in Sage 9.6.beta6

88e25dd77b Trac #33631: Github workflow: Fix pyright

78bfb6c7ad Updated SageMath version to 9.6.beta7

author 'Vincent Delecroix, Travis Scrimshaw,' does not look right

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 9635e1e to 4ed53f8

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

4ed53f8 Merge branch 'public/ticket/33497' of https://github.com/sagemath/sagetrac-mirror into public/ticket/33497

tscrim commented 2 years ago

comment:39

I am not getting any merge failure on 9.6.rc1. Although I guess this is probably too late to be merged into 9.6, so I guess we just have to wait for 9.7.beta0...

sagemath / sage

coerce vs _coerce_ vs _coerce_c #33497