Opened 7 years ago
Closed 6 years ago
#20767 closed enhancement (fixed)
Move coercion to Element
Reported by:  Jeroen Demeyer  Owned by:  

Priority:  major  Milestone:  sage7.4 
Component:  coercion  Keywords:  
Cc:  Nicolas M. Thiéry, Vincent Delecroix, Simon King  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Nicolas M. Thiéry 
Report Upstream:  N/A  Work issues:  
Branch:  25c9d7d (Commits, GitHub, GitLab)  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 7 years ago by
Description:  modified (diff) 

Summary:  Move coercion down to Element → Move coercion to Element 
comment:2 Changed 6 years ago by
Dependencies:  #269, #20740, #20753, #20761, #20757 → #269, #20740, #20753, #20761, #20757, #20686 

comment:3 Changed 6 years ago by
Dependencies:  #269, #20740, #20753, #20761, #20757, #20686 → #269, #20740, #20753, #20761, #20757, #20686, #20836 

comment:4 Changed 6 years ago by
Dependencies:  #269, #20740, #20753, #20761, #20757, #20686, #20836 → #269, #20740, #20753, #20761, #20757, #20686, #20836, #20852 

comment:5 Changed 6 years ago by
Branch:  → u/jdemeyer/move_coercion_to_element 

comment:6 Changed 6 years ago by
Commit:  → ce70b30b0e05473aead282439474a176f09abe6c 

Dependencies:  #269, #20740, #20753, #20761, #20757, #20686, #20836, #20852 → #269, #20686, #15947 
comment:7 Changed 6 years ago by
Dependencies:  #269, #20686, #15947 → #269, #20686 

comment:8 Changed 6 years ago by
Commit:  ce70b30b0e05473aead282439474a176f09abe6c → 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 6 years ago by
Commit:  04d1ef594face8e9cebd99acae4252a7b9899d38 → 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 6 years ago by
Commit:  ada2e74a6104e88da56b391e681500d3ebc88d3d → 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 6 years ago by
Commit:  b9a3614592e4a3a2f66ce2d2e52dcb0dcf013384 → 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 6 years ago by
Dependencies:  #269, #20686 → #21126, #20686 

comment:13 Changed 6 years ago by
Commit:  9c4bdefc8cbb6638699d620586d1bb707817094d → e4919e05c644722170152603e6de769f201d5478 

comment:14 Changed 6 years ago by
Dependencies:  #21126, #20686 → #21126, #20686, #21139 

comment:15 Changed 6 years ago by
Commit:  e4919e05c644722170152603e6de769f201d5478 → 1570a11911c8812f648369981182760767005cf3 

comment:16 Changed 6 years ago by
Dependencies:  #21126, #20686, #21139 → #21126, #20686, #21139, #21140 

comment:17 Changed 6 years ago by
Commit:  1570a11911c8812f648369981182760767005cf3 → 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 6 years ago by
Commit:  950d4ab0b6be3adfcb47b738dfc74a8b931b6cf1 → 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 6 years ago by
Commit:  7c7f7293f38f324e77530c3fc565cf3e2a839458 → 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 6 years ago by
Commit:  4d82fb71ce86a9eb79a41595571f1105f13aee36 → 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 6 years ago by
Commit:  a67c31fc539dc21188dd36e5ce972be034d93b9f → 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 6 years ago by
Commit:  bbc6e049de5b9376f9fb8f37cf23850529448c5a → 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 6 years ago by
Commit:  59f29566265b7ef0f559bf5fd95232c99e3f3a27 → 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 6 years ago by
Dependencies:  #21126, #20686, #21139, #21140 → #21126, #20686, #21139, #21140, #21152 

comment:25 Changed 6 years ago by
Commit:  8fc8d924e856d831ef21451aed4f687cb0574c4d → 21aea97e09005d62513b08c639ae086aa157af80 

comment:26 Changed 6 years ago by
Dependencies:  #21126, #20686, #21139, #21140, #21152 → #21126, #20686, #21139, #21140, #21152, #21153, #21154 

comment:27 Changed 6 years ago by
Commit:  21aea97e09005d62513b08c639ae086aa157af80 → 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 6 years ago by
Authors:  → Jeroen Demeyer 

Status:  new → needs_review 
comment:29 Changed 6 years ago by
Commit:  16c545efb9c09c7054ce7b7638312fcd36a55227 → 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 6 years ago by
Description:  modified (diff) 

comment:31 Changed 6 years ago by
Description:  modified (diff) 

comment:32 Changed 6 years ago by
Description:  modified (diff) 

comment:33 Changed 6 years ago by
Description:  modified (diff) 

comment:34 Changed 6 years ago by
Cc:  Simon King added 

comment:35 Changed 6 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:37 Changed 6 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 6 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 6 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 6 years ago by
Milestone:  sage7.3 → sage7.4 

Status:  needs_review → needs_work 
Doctest failures with 7.4.beta2.
comment:41 Changed 6 years ago by
Commit:  e1d2ba46574273ec4d358e654ed195a82b81a654 → 343131a9951dc842c462809b9366b13416382dfc 

comment:42 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:43 Changed 6 years ago by
Description:  modified (diff) 

comment:44 Changed 6 years ago by
Branch:  u/jdemeyer/move_coercion_to_element → u/nthiery/move_coercion_to_element 

comment:45 Changed 6 years ago by
Branch:  u/nthiery/move_coercion_to_element → u/jdemeyer/move_coercion_to_element 

comment:46 Changed 6 years ago by
Commit:  343131a9951dc842c462809b9366b13416382dfc → 59a04e46a93e6c0b4fe811fabf9c8eee7ed8e3e8 

comment:47 Changed 6 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 6 years ago by
Branch:  u/jdemeyer/move_coercion_to_element → u/nthiery/move_coercion_to_element 

comment:49 Changed 6 years ago by
Commit:  59a04e46a93e6c0b4fe811fabf9c8eee7ed8e3e8 → 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 6 years ago by
Branch:  u/nthiery/move_coercion_to_element → u/jdemeyer/move_coercion_to_element 

comment:51 Changed 6 years ago by
Commit:  2d409484e23387d98d8a697aaac76a70017f53e8 → 25c9d7d6effbc50e9aeb0d7e8a7e18d92da30aa2 

Branch pushed to git repo; I updated commit sha1. New commits:
25c9d7d  Add tests for _add_long and _mul_long

comment:53 followups: 54 56 Changed 6 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 Changed 6 years ago by
Reviewers:  → Nicolas M. Thiéry 

Status:  needs_review → 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 6 years ago by
Dependencies:  #21126, #20686, #21139, #21140, #21152, #21153, #21154 → #21126, #20686, #21139, #21140, #21152, #21153, #21154, #21441 

This ticket is suffering also from #21441 (see patchbot)
comment:56 Changed 6 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:58 Changed 6 years ago by
Branch:  u/jdemeyer/move_coercion_to_element → 25c9d7d6effbc50e9aeb0d7e8a7e18d92da30aa2 

Resolution:  → fixed 
Status:  positive_review → 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