Move coercion to Element

[jdemeyer]

This ticket has 2 main changes:

1. Move all coercion logic from RingElement, ModuleElement and the like to Element.

Because of this, it matters a lot less whether a class inherits from Element or from ModuleElement/RingElement/FieldElement...

One difference remains: the more specialized classes have some default implementations for arithmetic. For example, ModuleElement implements unary negation as multiplication by -1. The base class Element has no such default implementations.

This patch also affects lookup in categories: with this patch, double-underscore methods like __add__ are never taken from the category. The Element classes take precedence over the category, so the default implementations of arithmetic operations will override whatever is in the category (this is existing behaviour, not affected by this patch). For the base class Element, this is not an issue since there are no default implementations.

2. Change the implementation of double-underscore methods like __add__ to return NotImplemented (rather than raise an error) if one argument is not a Sage Element and coercion fails.

This will cause Python to try the reversed operation (__radd__ or __add__ in Cython). This way, Sage has improved support for operations with non-Sage types.

Depends on #21126
Depends on #20686
Depends on #21139
Depends on #21140
Depends on #21152
Depends on #21153
Depends on #21154
Depends on #21441

CC: @nthiery @videlec @simon-king-jena

Component: coercion

Author: Jeroen Demeyer

Branch/Commit: 25c9d7d

Reviewer: Nicolas M. Thiéry

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

[jdemeyer]

Changed dependencies from #269, #20740, #20753, #20761, #20757 to #269, #20740, #20753, #20761, #20757, #20686

[jdemeyer]

Changed dependencies from #269, #20740, #20753, #20761, #20757, #20686 to #269, #20740, #20753, #20761, #20757, #20686, #20836

[jdemeyer]

Changed dependencies from #269, #20740, #20753, #20761, #20757, #20686, #20836 to #269, #20740, #20753, #20761, #20757, #20686, #20836, #20852

[jdemeyer]

Branch: u/jdemeyer/move_coercion_to_element

[jdemeyer]

New commits:

9a1ff6e	Improve getattr_from_other_class
744ffa6	Test that static methods work
f5361d9	Improve _sage_src_lines_()
8079508	Implement `__mod__` in the coercion model
b7856d5	Merge commit 'f5361d972a61dcb60a76425264baa9e6d134e54b'; commit 'ccd5fcc16ac1708854a600cfc259c23d6c7568ff'; commit 'c41bc91addb41f593e4509d3fd6e9ce7c5789b39'; commit '80795088d0774413d5cbb94459d95b430e9fe2ad' into t/20767/move_coercion_to_element
dc84d88	Remove comments about "private" attributes with two underscores
8a4f48d	Abstract away category lookup in cdef method getattr_from_category
20c8675	Merge remote-tracking branch 'origin/u/jdemeyer/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes' into t/20767/move_coercion_to_element
ce70b30	Move arithmetic using coercion model to Element

[jdemeyer]

Commit: ce70b30

[jdemeyer]

Changed dependencies from #269, #20740, #20753, #20761, #20757, #20686, #20836, #20852 to #269, #20686, #15947

[jdemeyer]

Changed dependencies from #269, #20686, #15947 to #269, #20686

[sagetrac-git]

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

88d9757	Comment that getattr_from_other_class is cached
7140ee3	Prefer absolute imports
fc6487c	Merge remote-tracking branch 'trac/u/jdemeyer/upgrade_to_cython_0_24_1' into t/20686/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes
16d4743	EvaluationMethods should be a new-style class
54ccd82	Remove `__dict__` attribute from ElementWrapper
9a71db1	Merge commit '54ccd828cb20b1254aeaf55bf7edab4a73f1032a'; tag '7.3.beta9' into t/20767/move_coercion_to_element
04d1ef5	Move arithmetic using coercion model to Element

[sagetrac-git]

Changed commit from ce70b30 to 04d1ef5

[sagetrac-git]

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

ada2e74

Move arithmetic using coercion model to Element

[sagetrac-git]

Changed commit from 04d1ef5 to ada2e74

[sagetrac-git]

Changed commit from ada2e74 to b9a3614

[sagetrac-git]

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

b9a3614

Move arithmetic using coercion model to Element

[sagetrac-git]

Changed commit from b9a3614 to 9c4bdef

[sagetrac-git]

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

9c4bdef

Move arithmetic using coercion model to Element

[jdemeyer]

Changed dependencies from #269, #20686 to #21126, #20686

[sagetrac-git]

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

093395e	Add a Cython fix for cpdef methods
972533a	Merge commit '54ccd828cb20b1254aeaf55bf7edab4a73f1032a' into t/20767/move_coercion_to_element
e4919e0	Move arithmetic using coercion model to Element

[sagetrac-git]

Changed commit from 9c4bdef to e4919e0

[jdemeyer]

Changed dependencies from #21126, #20686 to #21126, #20686, #21139

[sagetrac-git]

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

b72af0c	Implement negation for modular forms
1570a11	Move arithmetic using coercion model to Element

[sagetrac-git]

Changed commit from e4919e0 to 1570a11

[jdemeyer]

Changed dependencies from #21126, #20686, #21139 to #21126, #20686, #21139, #21140

[sagetrac-git]

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

a0d5ec6	Implement negation for modular forms
fbcda3b	Merge branch 't/21139/implement_negation_for_modular_forms' into t/20767/move_coercion_to_element
69d18c7	Remove redundant `_lmul_` and `_rmul_` methods
2f8e1c9	Merge commit '69d18c75875c8a9f770e9ef493d6e721854f4448' into t/20767/move_coercion_to_element
950d4ab	Move arithmetic using coercion model to Element

[nthiery]

Changed branch from u/jdemeyer/move_coercion_to_element to u/nthiery/move_coercion_to_element

[nthiery]

Changed commit from 59a04e4 to 2d40948

[nthiery]

comment:49

I read your new documentation; that's a very nice addition to the Sage documentation. I have done some changes here and there, and added a draft of explanation of the cdef _add_ trick. Please proofread. The _add_long and _mul_long protocols need to be documented and tested in element.pyx; see the TODO there.

Other than this, I believe this is good to go. Thanks a lot Jeroen!

---

New commits:

2d40948

20767: Proofreading doc + doc of cdef `_add_` trick + TODO about _add_long / _mul_long

[jdemeyer]

Changed branch from u/nthiery/move_coercion_to_element to u/jdemeyer/move_coercion_to_element

[sagetrac-git]

Changed commit from 2d40948 to 25c9d7d

[sagetrac-git]

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

25c9d7d

Add tests for _add_long and _mul_long

[jdemeyer]

comment:52

Done, needs review.

[nthiery]

comment:53

Thanks; I checked your commits and am happy with them.

Two small questions:

• The documentation currently seems to suggest that it's only possible to implement _add_long and _mul_long within a Cython class. Is that right? Of course, in a Python class it's probably rarely relevant to implement _add_long, but if this works there is no reason to hide it in the doc.

• Concrete implementations (of `_add_`) in subclasses should be cpdefordef methods: do we need to be specific? cdef would actually work too, right? Or is it because with cdef, a Python level call to x._add_(y) would fail?

Once you have made up your mind on the above, you can set a positive review on my behalf (assuming of course all tests keep passing).

Thanks!

Cheers,
Nicolas

[jdemeyer]

Reviewer: Nicolas M. Thiéry

[jdemeyer]

comment:54

Replying to @nthiery:

• The documentation currently seems to suggest that it's only possible to implement _add_long and _mul_long within a Cython class. Is that right?

Yes, it's a cdef method, so it's Cython-only. Since it is only an optimization, it makes little sense to allow it to be a Python method. If you care about performance, you should be using Cython anyway.

• Concrete implementations (of `_add_`) in subclasses should be cpdefordef methods: do we need to be specific? cdef would actually work too, right?
  Or is it because with cdef, a Python level call to x._add_(y) would fail?

It would work for x + y yes. But I think it is expected that x._add_(y) also works. Anyway, that is how things were historically.

Maybe one could argue that x._add_(y) should not be required to work from Python. But let's not change that in this ticket.

[jdemeyer]

Changed dependencies from #21126, #20686, #21139, #21140, #21152, #21153, #21154 to #21126, #20686, #21139, #21140, #21152, #21153, #21154, #21441

[jdemeyer]

comment:55

This ticket is suffering also from #21441 (see patchbot)

[jdemeyer]

comment:56

Replying to @nthiery:

• Concrete implementations (of _add_) in subclasses should be cpdef or def methods``: do we need to be specific? cdef would actually work too, right? Or is it because with cdef, a Python level call to x._add_(y) would fail?

A more complete answer: yes, they really should be cpdef or def methods because you want to allow Python subclasses to override them. A cdef method is Cython-only and cannot be overridden by a Python method. Of course, Element._add_ is cdef but it "manually" checks for a Python _add_ method.

[nthiery]

comment:57

Ah, yes, That's a good point indeed. Thanks!

[vbraun]

Changed branch from u/jdemeyer/move_coercion_to_element to 25c9d7d