search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.22k stars 424 forks source link

Rename coercion action methods #5597

Closed robertwb closed 14 years ago

robertwb commented 15 years ago 
Currently, if A has an action on B (where B is not an A-module) one  
implements either a._l_action_ or b._r_action_. This is because  
sometimes it makes sense to put the method on the actor (e.g. Galois  
groups acting on field elements) and sometimes on the acted on (e.g.  
matrices acting on quadratic forms). However, the _x_action_ is hard  
to remember and doesn't always correspond to right/left actions. This  
may be why they're hardly used up to this point.

The proposal is to make the methods a._act_on_(b, self_on_left) and  
b._acted_upon_(a, self_on_left). In other words, a*b would try  
"a._act_on_(b, True)" and "b._acted_upon_(a, False)".

See discussion at

http://groups.google.com/group/sage-devel/browse_thread/thread/4c6ce1731ace1016

CC: @nthiery @sagetrac-GeorgSWeber @craigcitro

Component: coercion

Keywords: actions, left actions, right actions

Author: Robert Bradshaw

Reviewer: Nicolas M. Thiéry

Merged: sage-4.2.alpha1

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

robertwb commented 15 years ago

Attachment: 5597-coerce-actions.patch.gz

robertwb commented 15 years ago
comment:1

Rename and cleanup actions. Depends on #5596.

4ecb6cfd-27aa-477a-80a8-c49e29e5d355 commented 15 years ago
comment:2

Minor issue: in element.pyx, both new actions are commented with "Use this method to implement self acting on x." --- probably a copy'n'paste error for "_actedupon"?!

Cheers, gsw

williamstein commented 15 years ago
comment:3

REFEREE REPORT:

This patch contains substantial new code that has no doctests. Please post another patch with full coverage.

robertwb commented 14 years ago

rebased against 4.1.1

robertwb commented 14 years ago

Attachment: 5597-coerce-actions-new.patch.gz

robertwb commented 14 years ago
comment:5

Attachment: 5597-coerce-actions-examples.patch.gz

robertwb commented 14 years ago

Attachment: 5597-referee-comments.patch.gz

nthiery commented 14 years ago
comment:6

Hi Robert,

Sorry for hopping so late in the discussion. I am not sure how I understand how left vs right actions are handled.

In a*b, are you always making the assumption that a is acting on b?

If I have an algebra B (whose code I don't want to touch), and implement a right B-module A, am I supposed to implement:

a.act_on(b)?

Or will a*b try all of:

b.act_on(a, False) b.acted_upon(a, False) a.act_on(b, True) a.acted_upon(b, True)

robertwb commented 14 years ago
comment:7

Yes, it should be trying all 4 of these options.

nthiery commented 14 years ago
comment:8

Replying to @robertwb:

Yes, it should be trying all 4 of these options.

Ok. Then I would prefer:

a.act_on_left(b) b.act_on_right(a) a.acted_upon_right(b) b.acted_upon_left(a)

which makes it easier to implement independently the left and right actions on a module, and possibly override just one or the other in a subclass.

That being said, we can leave things as is. Those modules for which left and right action do not coincide can later implement a.acted_upon(...) by delegating the work to acted_upon_left and acted_upon_right.

robertwb commented 14 years ago
comment:9

But then we're back to the same problem of s.acted_upon_right(p) not being obvious whether s or p was the one on the right (though it's a bit better). In any case, this behavior is easy to implement in a superclass.

So, is this a positive review (pending all doctests passing, which they did last I checked)?

nthiery commented 14 years ago

Reviewer: Nicolas M. Thiéry

nthiery commented 14 years ago

Author: Robert Bradshaw

nthiery commented 14 years ago
comment:10

Replying to @robertwb:

But then we're back to the same problem of s.acted_upon_right(p) not being obvious whether s or p was the one on the right (though it's a bit better).

It sounds rather clear to me.

In any case, this behavior is easy to implement in a superclass.

Yes.

So, is this a positive review (pending all doctests passing, which they did last I checked)?

Yes, I just wanted to discuss the matter first. Also, part of this may become obsolete once I will have a prototype implementation of overloaded operators/functions as in MuPAD, with a declarative interface (the Sage-combinat people need them anyway for other purposes). I'll post here a link to the appropriate ticket when times come.

nthiery commented 14 years ago

Changed keywords from none to actions, left actions, right actions

mwhansen commented 14 years ago

Attachment: trac_5597.patch.gz

Attachment: trac_5597-infinite_polynomial_ring.patch.gz

mwhansen commented 14 years ago
comment:12

I've attached trac_5597.patch which is all the the relevant patches folded together and then rebased.

I also attached trac_5597-infinite_polynomial_ring.patch which fixes some failures since an_element was returning a "generator" and not an actual element.

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago
comment:13

I applied just attachment: trac_5597-infinite_polynomial_ring.patch to 4.2.alpha0 and doctested sage/rings/polynomial/infinite_polynomial_ring.py. Positive review for this patch.

mwhansen commented 14 years ago

Merged: sage-4.2.alpha1

mwhansen commented 14 years ago
comment:14

Merged trac_5597.patch and trac_5597-infinite_polynomial_ring.patch in sage-4.2.alpha1.

robertwb commented 14 years ago
comment:15

Thanks! It's so good to finally see all these coercion and category patches go in.

mwhansen commented 14 years ago
comment:16

Me too! Thanks for doing them!