Opened 4 years ago
Closed 3 years ago
#20767 closed enhancement (fixed)
Move coercion to Element
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage7.4 
Component:  coercion  Keywords:  
Cc:  nthiery, vdelecroix, SimonKing  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Nicolas M. Thiéry 
Report Upstream:  N/A  Work issues:  
Branch:  25c9d7d (Commits)  Commit:  25c9d7d6effbc50e9aeb0d7e8a7e18d92da30aa2 
Dependencies:  #21126, #20686, #21139, #21140, #21152, #21153, #21154, #21441  Stopgaps: 
Description (last modified by )
This ticket has 2 main changes:
 Move all coercion logic from
RingElement
,ModuleElement
and the like toElement
.
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, doubleunderscore 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.
 Change the implementation of doubleunderscore methods like
__add__
to returnNotImplemented
(rather than raise an error) if one argument is not a SageElement
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 nonSage types.
Change History (58)
comment:1 Changed 4 years ago by
 Description modified (diff)
 Summary changed from Move coercion down to Element to Move coercion to Element
comment:2 Changed 3 years ago by
 Dependencies changed from #269, #20740, #20753, #20761, #20757 to #269, #20740, #20753, #20761, #20757, #20686
comment:3 Changed 3 years ago by
 Dependencies changed from #269, #20740, #20753, #20761, #20757, #20686 to #269, #20740, #20753, #20761, #20757, #20686, #20836
comment:4 Changed 3 years ago by
 Dependencies changed from #269, #20740, #20753, #20761, #20757, #20686, #20836 to #269, #20740, #20753, #20761, #20757, #20686, #20836, #20852
comment:5 Changed 3 years ago by
 Branch set to u/jdemeyer/move_coercion_to_element
comment:6 Changed 3 years ago by
 Commit set to ce70b30b0e05473aead282439474a176f09abe6c
 Dependencies changed from #269, #20740, #20753, #20761, #20757, #20686, #20836, #20852 to #269, #20686, #15947
comment:7 Changed 3 years ago by
 Dependencies changed from #269, #20686, #15947 to #269, #20686
comment:8 Changed 3 years ago by
 Commit changed from ce70b30b0e05473aead282439474a176f09abe6c to 04d1ef594face8e9cebd99acae4252a7b9899d38
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 remotetracking 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 newstyle 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

comment:9 Changed 3 years ago by
 Commit changed from 04d1ef594face8e9cebd99acae4252a7b9899d38 to ada2e74a6104e88da56b391e681500d3ebc88d3d
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ada2e74  Move arithmetic using coercion model to Element

comment:10 Changed 3 years ago by
 Commit changed from ada2e74a6104e88da56b391e681500d3ebc88d3d to b9a3614592e4a3a2f66ce2d2e52dcb0dcf013384
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b9a3614  Move arithmetic using coercion model to Element

comment:11 Changed 3 years ago by
 Commit changed from b9a3614592e4a3a2f66ce2d2e52dcb0dcf013384 to 9c4bdefc8cbb6638699d620586d1bb707817094d
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9c4bdef  Move arithmetic using coercion model to Element

comment:12 Changed 3 years ago by
 Dependencies changed from #269, #20686 to #21126, #20686
comment:13 Changed 3 years ago by
 Commit changed from 9c4bdefc8cbb6638699d620586d1bb707817094d to e4919e05c644722170152603e6de769f201d5478
comment:14 Changed 3 years ago by
 Dependencies changed from #21126, #20686 to #21126, #20686, #21139
comment:15 Changed 3 years ago by
 Commit changed from e4919e05c644722170152603e6de769f201d5478 to 1570a11911c8812f648369981182760767005cf3
comment:16 Changed 3 years ago by
 Dependencies changed from #21126, #20686, #21139 to #21126, #20686, #21139, #21140
comment:17 Changed 3 years ago by
 Commit changed from 1570a11911c8812f648369981182760767005cf3 to 950d4ab0b6be3adfcb47b738dfc74a8b931b6cf1
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

comment:18 Changed 3 years ago by
 Commit changed from 950d4ab0b6be3adfcb47b738dfc74a8b931b6cf1 to 7c7f7293f38f324e77530c3fc565cf3e2a839458
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7c7f729  Move arithmetic using coercion model to Element

comment:19 Changed 3 years ago by
 Commit changed from 7c7f7293f38f324e77530c3fc565cf3e2a839458 to 4d82fb71ce86a9eb79a41595571f1105f13aee36
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4d82fb7  Move arithmetic using coercion model to Element

comment:20 Changed 3 years ago by
 Commit changed from 4d82fb71ce86a9eb79a41595571f1105f13aee36 to a67c31fc539dc21188dd36e5ce972be034d93b9f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a67c31f  Move arithmetic using coercion model to Element

comment:21 Changed 3 years ago by
 Commit changed from a67c31fc539dc21188dd36e5ce972be034d93b9f to bbc6e049de5b9376f9fb8f37cf23850529448c5a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bbc6e04  Move arithmetic using coercion model to Element

comment:22 Changed 3 years ago by
 Commit changed from bbc6e049de5b9376f9fb8f37cf23850529448c5a to 59f29566265b7ef0f559bf5fd95232c99e3f3a27
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
59f2956  Move arithmetic using coercion model to Element

comment:23 Changed 3 years ago by
 Commit changed from 59f29566265b7ef0f559bf5fd95232c99e3f3a27 to 8fc8d924e856d831ef21451aed4f687cb0574c4d
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8fc8d92  Move arithmetic using coercion model to Element

comment:24 Changed 3 years ago by
 Dependencies changed from #21126, #20686, #21139, #21140 to #21126, #20686, #21139, #21140, #21152
comment:25 Changed 3 years ago by
 Commit changed from 8fc8d924e856d831ef21451aed4f687cb0574c4d to 21aea97e09005d62513b08c639ae086aa157af80
comment:26 Changed 3 years ago by
 Dependencies changed from #21126, #20686, #21139, #21140, #21152 to #21126, #20686, #21139, #21140, #21152, #21153, #21154
comment:27 Changed 3 years ago by
 Commit changed from 21aea97e09005d62513b08c639ae086aa157af80 to 16c545efb9c09c7054ce7b7638312fcd36a55227
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0a44b08  Remove one doctest for __mul__

54b0fcd  Various minor fixes for the coercion model

2405dde  Merge commit '0a44b082f08c742fbe564c5afdd2f7309fabbb52'; commit '54b0fcdaf2fd69cd64978da526af6774b57a59a8' into t/20767/move_coercion_to_element

16c545e  Move arithmetic using coercion model to Element

comment:28 Changed 3 years ago by
 Status changed from new to needs_review
comment:29 Changed 3 years ago by
 Commit changed from 16c545efb9c09c7054ce7b7638312fcd36a55227 to e1d2ba46574273ec4d358e654ed195a82b81a654
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e1d2ba4  Move arithmetic using coercion model to Element

comment:30 Changed 3 years ago by
 Description modified (diff)
comment:31 Changed 3 years ago by
 Description modified (diff)
comment:32 Changed 3 years ago by
 Description modified (diff)
comment:33 Changed 3 years ago by
 Description modified (diff)
comment:34 Changed 3 years ago by
 Cc SimonKing added
comment:35 Changed 3 years ago by
It would be interesting (to me) to see whether the coerce action business of #18756 can be used here as well.
comment:36 Changed 3 years ago by
... and of #18758
comment:37 Changed 3 years ago by
Well, this ticket deals with the "other side" of the coercion framework: it changes how the coercion model is called, not how the coercion model implements arithmetic. It seems like the other tickets do the opposite. So the tickets look related but also quite independent.
comment:38 Changed 3 years ago by
Hi
Just as a reminder: There are situation where we need ring element operations in a parent resp. for an element which is not really considered a ring element.
Example:
 Take a graded ring.
 Take two elements from different weightcomponents.
 Note that the elements by themself are considered elements of vector spaces (which is what we want because we probably want to do operations that only make sense for the vector space, etc).
 However we still want to be able to "add" _and_ "multiply" such elements.
 Add would result in a ring element (unless the components are the same).
 Multiply would result in a vector in a higher weightcomponent.
I had to deal with this when I implemented the graded rings of modular forms. See src/sage/modular/modform_hecketriangle/element.py for more details. There I solved it as follows: I derived the element from FormsRingElement? (so it has the multiply methods) but the parent is still just a vector space. I also made sure that arithmetic operations with elements end up in the correct space.
Will this still work with this ticket? Can you check the tests for this maybe?
Side note: At the moment I don't really have time to actively develop, so I can't really promise much help.
Best Jonas
comment:39 Changed 3 years ago by
This ticket should not cause many functional differences for existing code, except for the fact that doubleunderscore methods can no longer be implemented in categories and some other minor things, like using parent.one()
instead of 1
.
All doctests in Sage pass with this ticket (see for example the patchbot), so the code that you refer to should still work too.
comment:40 Changed 3 years ago by
 Milestone changed from sage7.3 to sage7.4
 Status changed from needs_review to needs_work
Doctest failures with 7.4.beta2.
comment:41 Changed 3 years ago by
 Commit changed from e1d2ba46574273ec4d358e654ed195a82b81a654 to 343131a9951dc842c462809b9366b13416382dfc
comment:42 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:43 Changed 3 years ago by
 Description modified (diff)
comment:44 Changed 3 years ago by
 Branch changed from u/jdemeyer/move_coercion_to_element to u/nthiery/move_coercion_to_element
comment:45 Changed 3 years ago by
 Branch changed from u/nthiery/move_coercion_to_element to u/jdemeyer/move_coercion_to_element
comment:46 Changed 3 years ago by
 Commit changed from 343131a9951dc842c462809b9366b13416382dfc to 59a04e46a93e6c0b4fe811fabf9c8eee7ed8e3e8
comment:47 Changed 3 years ago by
Nicolas, I took care of your comments. If you are missing something, feel free to notify me or to make the change yourself.
comment:48 Changed 3 years ago by
 Branch changed from u/jdemeyer/move_coercion_to_element to u/nthiery/move_coercion_to_element
comment:49 Changed 3 years ago by
 Commit changed from 59a04e46a93e6c0b4fe811fabf9c8eee7ed8e3e8 to 2d409484e23387d98d8a697aaac76a70017f53e8
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

comment:50 Changed 3 years ago by
 Branch changed from u/nthiery/move_coercion_to_element to u/jdemeyer/move_coercion_to_element
comment:51 Changed 3 years ago by
 Commit changed from 2d409484e23387d98d8a697aaac76a70017f53e8 to 25c9d7d6effbc50e9aeb0d7e8a7e18d92da30aa2
Branch pushed to git repo; I updated commit sha1. New commits:
25c9d7d  Add tests for _add_long and _mul_long

comment:52 Changed 3 years ago by
Done, needs review.
comment:53 followups: ↓ 54 ↓ 56 Changed 3 years ago by
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 becpdef
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?
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
comment:54 in reply to: ↑ 53 Changed 3 years ago by
 Reviewers set to Nicolas M. Thiéry
 Status changed from needs_review to positive_review
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 Cythononly. 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 becpdef
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?
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.
comment:55 Changed 3 years ago by
 Dependencies changed from #21126, #20686, #21139, #21140, #21152, #21153, #21154 to #21126, #20686, #21139, #21140, #21152, #21153, #21154, #21441
This ticket is suffering also from #21441 (see patchbot)
comment:56 in reply to: ↑ 53 Changed 3 years ago by
Replying to nthiery:
 Concrete implementations (of
_add_
) in subclasses should becpdef
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 Cythononly and cannot be overridden by a Python method. Of course, Element._add_
is cdef
but it "manually" checks for a Python _add_
method.
comment:57 Changed 3 years ago by
Ah, yes, That's a good point indeed. Thanks!
comment:58 Changed 3 years ago by
 Branch changed from u/jdemeyer/move_coercion_to_element to 25c9d7d6effbc50e9aeb0d7e8a7e18d92da30aa2
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Improve getattr_from_other_class
Test that static methods work
Improve _sage_src_lines_()
Implement __mod__ in the coercion model
Merge commit 'f5361d972a61dcb60a76425264baa9e6d134e54b'; commit 'ccd5fcc16ac1708854a600cfc259c23d6c7568ff'; commit 'c41bc91addb41f593e4509d3fd6e9ce7c5789b39'; commit '80795088d0774413d5cbb94459d95b430e9fe2ad' into t/20767/move_coercion_to_element
Remove comments about "private" attributes with two underscores
Abstract away category lookup in cdef method getattr_from_category
Merge remotetracking branch 'origin/u/jdemeyer/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes' into t/20767/move_coercion_to_element
Move arithmetic using coercion model to Element