Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coerce vs _coerce_ vs _coerce_c #33497

Closed
videlec opened this issue Mar 13, 2022 · 53 comments
Closed

coerce vs _coerce_ vs _coerce_c #33497

videlec opened this issue Mar 13, 2022 · 53 comments

Comments

@videlec
Copy link
Contributor

videlec commented Mar 13, 2022

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 videlec added this to the sage-9.6 milestone Mar 13, 2022
@videlec

This comment has been minimized.

@videlec videlec changed the title coerce vs _coerce_ coerce vs _coerce_ vs _coerce_c Mar 13, 2022
@videlec

This comment has been minimized.

@videlec
Copy link
Contributor Author

videlec commented Mar 13, 2022

New commits:

cd7e89533497: _coerce_/_coerce_c -> coerce

@videlec
Copy link
Contributor Author

videlec commented Mar 13, 2022

Commit: cd7e895

@videlec
Copy link
Contributor Author

videlec commented Mar 13, 2022

Branch: u/vdelecroix/33497

@tscrim
Copy link
Collaborator

tscrim commented Mar 14, 2022

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
Copy link
Contributor Author

videlec commented Mar 14, 2022

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
Copy link
Collaborator

tscrim commented Mar 14, 2022

comment:6

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

@videlec
Copy link
Contributor Author

videlec commented Mar 14, 2022

comment:7

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

@tscrim
Copy link
Collaborator

tscrim commented Mar 14, 2022

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
Copy link
Contributor Author

videlec commented Mar 14, 2022

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
Copy link
Collaborator

tscrim commented Mar 14, 2022

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
Copy link
Contributor Author

videlec commented Mar 14, 2022

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
Copy link
Collaborator

tscrim commented Mar 14, 2022

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

comment:15

I have made #33525 for the Monsky problem.

@fchapoton
Copy link
Contributor

comment:16

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

@fchapoton
Copy link
Contributor

comment:17

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


New commits:

238a48233497: _coerce_/_coerce_c -> coerce

@fchapoton
Copy link
Contributor

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

@fchapoton
Copy link
Contributor

Changed commit from cd7e895 to 238a482

@fchapoton
Copy link
Contributor

Dependencies: #33525

@tscrim
Copy link
Collaborator

tscrim commented Mar 29, 2022

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

@tscrim
Copy link
Collaborator

tscrim commented Apr 1, 2022

comment:29

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

@tscrim
Copy link
Collaborator

tscrim commented Apr 5, 2022

comment:30

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

@videlec
Copy link
Contributor Author

videlec commented Apr 5, 2022

comment:31

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

@tscrim
Copy link
Collaborator

tscrim commented Apr 6, 2022

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
Copy link
Collaborator

tscrim commented Apr 6, 2022

Reviewer: Frédéric Chapoton, Travis Scrimshaw

@videlec
Copy link
Contributor Author

videlec commented Apr 7, 2022

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

@videlec
Copy link
Contributor Author

videlec commented Apr 7, 2022

comment:33

Looks good to me.

@videlec
Copy link
Contributor Author

videlec commented Apr 7, 2022

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

@tscrim
Copy link
Collaborator

tscrim commented Apr 8, 2022

comment:35

Thank you.

@vbraun
Copy link
Member

vbraun commented Apr 9, 2022

comment:36

Merge failure on top of:

5a999eb Trac #33468: Feature for gfan

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

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

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

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

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

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

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

a442962 Trac #33638: GH Actions: Fix cygwin

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

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

7acdc3d Trac #33246: canonical_label returns incorrect partite sets

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

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

b2ddb07 Trac #33635: fixing the broken linter, again

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

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

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

39d402c Trac #33629: add negative for continued fractions

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

00bc510 Trac #33560: pytest: ignore cython files

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

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

94a131c Trac #33612: sage.matrix.matrix_space: Rename a test_... function to test... (with deprecation)

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

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

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

b886d89 Trac #33628: Fig bug in graphs.RandomToleranceGraph

5ee8a50 Trac #33616: Simplify inventory builder

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

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

b8d5371 Trac #33576: Modernize coercion for SpecialHyperellipticQuotientRing

55da3e1 Trac #33572: sage -pytest

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

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

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

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

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

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

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

88e25dd Trac #33631: Github workflow: Fix pyright

78bfb6c Updated SageMath version to 9.6.beta7

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 25, 2022

Changed commit from 9635e1e to 4ed53f8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 25, 2022

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

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

@tscrim
Copy link
Collaborator

tscrim commented Apr 25, 2022

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...

@tscrim
Copy link
Collaborator

tscrim commented Apr 25, 2022

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 25, 2022

comment:40

The failure was actually just "author 'Vincent Delecroix, Travis Scrimshaw,' does not look right". You just fixed that.

@vbraun
Copy link
Member

vbraun commented May 20, 2022

Changed branch from public/ticket/33497 to 4ed53f8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants