Opened 4 years ago

Closed 3 years ago

#20767 closed enhancement (fixed)

Move coercion to Element

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.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 nthiery)

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.

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

Change History (58)

comment:1 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Move coercion down to Element to Move coercion to Element

comment:2 Changed 3 years ago by jdemeyer

  • Dependencies changed from #269, #20740, #20753, #20761, #20757 to #269, #20740, #20753, #20761, #20757, #20686

comment:3 Changed 3 years ago by jdemeyer

  • Dependencies changed from #269, #20740, #20753, #20761, #20757, #20686 to #269, #20740, #20753, #20761, #20757, #20686, #20836

comment:4 Changed 3 years ago by jdemeyer

  • 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 jdemeyer

  • Branch set to u/jdemeyer/move_coercion_to_element

comment:6 Changed 3 years ago by jdemeyer

  • Commit set to ce70b30b0e05473aead282439474a176f09abe6c
  • Dependencies changed from #269, #20740, #20753, #20761, #20757, #20686, #20836, #20852 to #269, #20686, #15947

New commits:

9a1ff6eImprove getattr_from_other_class
744ffa6Test that static methods work
f5361d9Improve _sage_src_lines_()
8079508Implement __mod__ in the coercion model
b7856d5Merge commit 'f5361d972a61dcb60a76425264baa9e6d134e54b'; commit 'ccd5fcc16ac1708854a600cfc259c23d6c7568ff'; commit 'c41bc91addb41f593e4509d3fd6e9ce7c5789b39'; commit '80795088d0774413d5cbb94459d95b430e9fe2ad' into t/20767/move_coercion_to_element
dc84d88Remove comments about "private" attributes with two underscores
8a4f48dAbstract away category lookup in cdef method getattr_from_category
20c8675Merge remote-tracking branch 'origin/u/jdemeyer/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes' into t/20767/move_coercion_to_element
ce70b30Move arithmetic using coercion model to Element

comment:7 Changed 3 years ago by jdemeyer

  • Dependencies changed from #269, #20686, #15947 to #269, #20686

comment:8 Changed 3 years ago by git

  • Commit changed from ce70b30b0e05473aead282439474a176f09abe6c to 04d1ef594face8e9cebd99acae4252a7b9899d38

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

88d9757Comment that getattr_from_other_class is cached
7140ee3Prefer absolute imports
fc6487cMerge 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
16d4743EvaluationMethods should be a new-style class
54ccd82Remove __dict__ attribute from ElementWrapper
9a71db1Merge commit '54ccd828cb20b1254aeaf55bf7edab4a73f1032a'; tag '7.3.beta9' into t/20767/move_coercion_to_element
04d1ef5Move arithmetic using coercion model to Element

comment:9 Changed 3 years ago by git

  • Commit changed from 04d1ef594face8e9cebd99acae4252a7b9899d38 to ada2e74a6104e88da56b391e681500d3ebc88d3d

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

ada2e74Move arithmetic using coercion model to Element

comment:10 Changed 3 years ago by git

  • Commit changed from ada2e74a6104e88da56b391e681500d3ebc88d3d to b9a3614592e4a3a2f66ce2d2e52dcb0dcf013384

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

b9a3614Move arithmetic using coercion model to Element

comment:11 Changed 3 years ago by git

  • Commit changed from b9a3614592e4a3a2f66ce2d2e52dcb0dcf013384 to 9c4bdefc8cbb6638699d620586d1bb707817094d

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

9c4bdefMove arithmetic using coercion model to Element

comment:12 Changed 3 years ago by jdemeyer

  • Dependencies changed from #269, #20686 to #21126, #20686

comment:13 Changed 3 years ago by git

  • Commit changed from 9c4bdefc8cbb6638699d620586d1bb707817094d to e4919e05c644722170152603e6de769f201d5478

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

093395eAdd a Cython fix for cpdef methods
972533aMerge commit '54ccd828cb20b1254aeaf55bf7edab4a73f1032a' into t/20767/move_coercion_to_element
e4919e0Move arithmetic using coercion model to Element

comment:14 Changed 3 years ago by jdemeyer

  • Dependencies changed from #21126, #20686 to #21126, #20686, #21139

comment:15 Changed 3 years ago by git

  • Commit changed from e4919e05c644722170152603e6de769f201d5478 to 1570a11911c8812f648369981182760767005cf3

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

b72af0cImplement negation for modular forms
1570a11Move arithmetic using coercion model to Element

comment:16 Changed 3 years ago by jdemeyer

  • Dependencies changed from #21126, #20686, #21139 to #21126, #20686, #21139, #21140

comment:17 Changed 3 years ago by git

  • Commit changed from 1570a11911c8812f648369981182760767005cf3 to 950d4ab0b6be3adfcb47b738dfc74a8b931b6cf1

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

a0d5ec6Implement negation for modular forms
fbcda3bMerge branch 't/21139/implement_negation_for_modular_forms' into t/20767/move_coercion_to_element
69d18c7Remove redundant _lmul_ and _rmul_ methods
2f8e1c9Merge commit '69d18c75875c8a9f770e9ef493d6e721854f4448' into t/20767/move_coercion_to_element
950d4abMove arithmetic using coercion model to Element

comment:18 Changed 3 years ago by git

  • Commit changed from 950d4ab0b6be3adfcb47b738dfc74a8b931b6cf1 to 7c7f7293f38f324e77530c3fc565cf3e2a839458

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

7c7f729Move arithmetic using coercion model to Element

comment:19 Changed 3 years ago by git

  • Commit changed from 7c7f7293f38f324e77530c3fc565cf3e2a839458 to 4d82fb71ce86a9eb79a41595571f1105f13aee36

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

4d82fb7Move arithmetic using coercion model to Element

comment:20 Changed 3 years ago by git

  • Commit changed from 4d82fb71ce86a9eb79a41595571f1105f13aee36 to a67c31fc539dc21188dd36e5ce972be034d93b9f

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

a67c31fMove arithmetic using coercion model to Element

comment:21 Changed 3 years ago by git

  • Commit changed from a67c31fc539dc21188dd36e5ce972be034d93b9f to bbc6e049de5b9376f9fb8f37cf23850529448c5a

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

bbc6e04Move arithmetic using coercion model to Element

comment:22 Changed 3 years ago by git

  • Commit changed from bbc6e049de5b9376f9fb8f37cf23850529448c5a to 59f29566265b7ef0f559bf5fd95232c99e3f3a27

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

59f2956Move arithmetic using coercion model to Element

comment:23 Changed 3 years ago by git

  • Commit changed from 59f29566265b7ef0f559bf5fd95232c99e3f3a27 to 8fc8d924e856d831ef21451aed4f687cb0574c4d

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

8fc8d92Move arithmetic using coercion model to Element

comment:24 Changed 3 years ago by jdemeyer

  • Dependencies changed from #21126, #20686, #21139, #21140 to #21126, #20686, #21139, #21140, #21152

comment:25 Changed 3 years ago by git

  • Commit changed from 8fc8d924e856d831ef21451aed4f687cb0574c4d to 21aea97e09005d62513b08c639ae086aa157af80

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

12d9d32Implement unary operations in interfaces
aae48bdMerge branch 'ticket/21152' into t/20767/move_coercion_to_element
21aea97Move arithmetic using coercion model to Element

comment:26 Changed 3 years ago by jdemeyer

  • Dependencies changed from #21126, #20686, #21139, #21140, #21152 to #21126, #20686, #21139, #21140, #21152, #21153, #21154

comment:27 Changed 3 years ago by git

  • Commit changed from 21aea97e09005d62513b08c639ae086aa157af80 to 16c545efb9c09c7054ce7b7638312fcd36a55227

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

0a44b08Remove one doctest for __mul__
54b0fcdVarious minor fixes for the coercion model
2405ddeMerge commit '0a44b082f08c742fbe564c5afdd2f7309fabbb52'; commit '54b0fcdaf2fd69cd64978da526af6774b57a59a8' into t/20767/move_coercion_to_element
16c545eMove arithmetic using coercion model to Element

comment:28 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Status changed from new to needs_review

comment:29 Changed 3 years ago by git

  • Commit changed from 16c545efb9c09c7054ce7b7638312fcd36a55227 to e1d2ba46574273ec4d358e654ed195a82b81a654

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

e1d2ba4Move arithmetic using coercion model to Element

comment:30 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:31 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:32 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:33 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:34 Changed 3 years ago by jdemeyer

  • Cc SimonKing added

comment:35 Changed 3 years ago by SimonKing

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 SimonKing

... and of #18758

comment:37 Changed 3 years ago by jdemeyer

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 jj

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 weight-components.
  • 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 weight-component.

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 jdemeyer

This ticket should not cause many functional differences for existing code, except for the fact that double-underscore 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 jdemeyer

  • Milestone changed from sage-7.3 to sage-7.4
  • Status changed from needs_review to needs_work

Doctest failures with 7.4.beta2.

comment:41 Changed 3 years ago by git

  • Commit changed from e1d2ba46574273ec4d358e654ed195a82b81a654 to 343131a9951dc842c462809b9366b13416382dfc

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

7bfd42aMerge tag '7.4.beta2' into t/20767/move_coercion_to_element
343131aFix doctest for Trac #20767

comment:42 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:43 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:44 Changed 3 years ago by nthiery

  • Branch changed from u/jdemeyer/move_coercion_to_element to u/nthiery/move_coercion_to_element

comment:45 Changed 3 years ago by jdemeyer

  • Branch changed from u/nthiery/move_coercion_to_element to u/jdemeyer/move_coercion_to_element

comment:46 Changed 3 years ago by git

  • Commit changed from 343131a9951dc842c462809b9366b13416382dfc to 59a04e46a93e6c0b4fe811fabf9c8eee7ed8e3e8

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

2e8f9e4Return exception instead of error message
59a04e4Improve documentation and tests

comment:47 Changed 3 years ago by jdemeyer

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 nthiery

  • Branch changed from u/jdemeyer/move_coercion_to_element to u/nthiery/move_coercion_to_element

comment:49 Changed 3 years ago by nthiery

  • 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:

2d4094820767: Proofreading doc + doc of cdef _add_ trick + TODO about _add_long / _mul_long

comment:50 Changed 3 years ago by jdemeyer

  • Branch changed from u/nthiery/move_coercion_to_element to u/jdemeyer/move_coercion_to_element

comment:51 Changed 3 years ago by git

  • Commit changed from 2d409484e23387d98d8a697aaac76a70017f53e8 to 25c9d7d6effbc50e9aeb0d7e8a7e18d92da30aa2

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

25c9d7dAdd tests for _add_long and _mul_long

comment:52 Changed 3 years ago by jdemeyer

Done, needs review.

comment:53 follow-ups: Changed 3 years ago by nthiery

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

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 jdemeyer

  • 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 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 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?

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 jdemeyer

  • 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 jdemeyer

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.

comment:57 Changed 3 years ago by nthiery

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

comment:58 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/move_coercion_to_element to 25c9d7d6effbc50e9aeb0d7e8a7e18d92da30aa2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.