Opened 4 years ago

Closed 4 years ago

#14249 closed defect (fixed)

There should be no need to have _an_element_ to multiply elements

Reported by: SimonKing Owned by: robertwb
Priority: major Milestone: sage-5.10
Component: coercion Keywords:
Cc: nthiery Merged in: sage-5.10.rc0
Authors: Simon King Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14264 Stopgaps:

Description (last modified by SimonKing)

We currently have

  sage: from sage.structure.element import ModuleElement
  sage: class MyElement(ModuleElement):
  ....:     def __init__(self, x, y, parent=None):
  ....:         ModuleElement.__init__(self, parent)
  ....:     def _mul_(self, other):
  ....:         return self
  ....:     def _rmul_(self, other):
  ....:         return self
  ....:     def _lmul_(self, other):
  ....:         return self
  ....:
  sage: class MyParent(Parent):
  ....:     Element = MyElement
  ....:
  sage: P = MyParent(category=Rings())
  sage: P(1,2)
  Generic element of a structure
  sage: a = _
  sage: a*2
  Traceback (most recent call last):
  ...
  NotImplementedError: please implement _an_element_ for <class '__main__.MyParent_with_category'>

I find this very annoying.

The background is that the coercion model tries to get a multiplication action, namely RightModuleAction. During initialisation of the module action, some sanity tests are performed. In particular, an element of the acting parent and the acted-upon set are taken and the action called on these two elements.

Problem: Where to get the two elements from? Currently, they are gotten from the method an_element(), which in turn relies on _an_element_() being implemented if the default implementation is not good enough (which is the case in the example above).

But normally, we do not want to have the RightModuleAction just out of the blue: Typically (in the example above, at least), we want to create it during the first multiplication. And in that moment we do have two elements.

So, I propose to make it possible to pass the two elements being multiplied to the constructor of Left/RightModuleAction, and let an_element() be called only if no element is passed.

Apply

Attachments (2)

trac14249-fix_doc.patch (3.7 KB) - added by SimonKing 4 years ago.
Fixes according to Travis' remarks
trac_14249-coercion_without_an_element.patch (19.6 KB) - added by SimonKing 4 years ago.
Combined patch

Download all attachments as: .zip

Change History (19)

comment:1 Changed 4 years ago by SimonKing

  • Authors set to Simon King
  • Dependencies set to #14264
  • Status changed from new to needs_review

I am afraid that my patch touches parent.pxd and thus forces rebuilding most of Sage.

Anyway. If an action of one parent on some object is requested during arithmetic operations, then the coercion model will now use the two given elements to construct/test an action, rather than calling an_element() of the parent and the object, which might not always be available.

My patch uncovered a bug in sage/schemes/hyperelliptic_curves/jacobian_morphism.py: An IntegerMulAction was to be created, but for testing it, m+(-m) is attempted for the given element m. But the involved element used to raise an error on -m.

So, that's an independent bug, which I fixed in #14264.

comment:2 follow-up: Changed 4 years ago by nbruin

Isn't it a logical flaw that there should even be elements for an action to exist? I think it's entirely possible to have a group acting on an empty set. In fact, in number theory these things often arise. If C is a smooth projective genus 1 curve then it is a torsor under its jacobian E. This gives a functorial way for E(k) to act on C(k), for any extension k of the field of definition. In the interesting cases, C(k) is empty, so you have a finitely generated group E(k) acting on the empty set C(k).

This particular scenario isn't particularly relevant for the coercion framework, but it does show that actions on empty sets are natural to consider, so if we're not supporting that, there may be something wrong with our model.

Can't we just ditch the sanity check (or skip it if an element isn't easily obtained)?

comment:3 in reply to: ↑ 2 Changed 4 years ago by SimonKing

Replying to nbruin:

Isn't it a logical flaw that there should even be elements for an action to exist?

For general actions I agree. But:

This particular scenario isn't particularly relevant for the coercion framework, but it does show that actions on empty sets are natural to consider, so if we're not supporting that, there may be something wrong with our model.

Don't forget that what we are talking about here is in sage.structure.coerce_actions. So, this is for coercions, and it is to be applied to elements.

sage.categories.action is of course more general.

Can't we just ditch the sanity check (or skip it if an element isn't easily obtained)?

Part of the problem is that we have different ways to define an action on elements. There is _rmul_/_lmul_, there is _l_action/_r_action (at least according to sage.structure.coerce_actions, but it isn't used in sage.structure.coerce), there is _act_on_, and there is _acted_upon_. Moreover, there is IntegerMulAction. And since they are all acting on elements, I think it somehow does make sense to test which of the different flavours is available for the elements.

comment:4 Changed 4 years ago by SimonKing

That said, it is possible for a parent to return an action on request (implement _get_action_). Here, I think it does make sense to offer an optional parameter check, such that the elements are only tested if check=True (which should be the default, IMHO).

And my patch does introduce such a check parameter!

comment:5 Changed 4 years ago by SimonKing

  • Cc nthiery added

Since this is a prerequisite for #14279 (which would implement the coercion model for homsets), I am putting Nicolas on Cc. Needs review, hinthint...

comment:6 Changed 4 years ago by nthiery

I agree, basic arithmetic should not require to construct an_element. In general the coercion model's approach of monkey calling a method with garbage to test if by chance it would accept that garbage is ugly and fragile; I am in favor of any step away of it.

That's a first step :-)

I went through the patch, and it sounds reasonable. So if all test pass, I am fine with it.

comment:7 Changed 4 years ago by tscrim

Hey Simon,

Some minor things:

  • Line 78 of parent.pyx needs the double colon ::.
  • Could you use the python 3 syntax for the exceptions on lines 332 and 336 of coerce_actions.pyx? (raise CoercionException("msg"))
  • Use the new line continuations ....: in parent.pyx.

Thanks,
Travis

Changed 4 years ago by SimonKing

Fixes according to Travis' remarks

comment:8 Changed 4 years ago by SimonKing

I have added a new patch that hopefully succeeds in addressing Travis' comments.

comment:9 follow-up: Changed 4 years ago by SimonKing

The patchbot still complains about the old style line continuations in the first patch. However, the patchbot does not realise that the second patch changes these into new style line continuations. So, I guess the ticket is still ready for review...

comment:10 Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

Sorry for falling (slightly) behind on reviewing this. From looking at the patch, it looks good. I'll test it later tonight due to the recompile, promise.

comment:11 in reply to: ↑ 9 Changed 4 years ago by nthiery

Replying to SimonKing:

The patchbot still complains about the old style line continuations in the first patch. However, the patchbot does not realise that the second patch changes these into new style line continuations. So, I guess the ticket is still ready for review...

One workaround would be to fold the two patches into the first one, and leave the second one around for review.

comment:12 Changed 4 years ago by SimonKing

  • Description modified (diff)

OK, the patches got folded.

Apply trac_14249-coercion_without_an_element.patch

comment:13 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

Looks good to me. Thanks Simon.

comment:14 Changed 4 years ago by robertwb

Note that a failing plugin is not necessarily a blocker for positive review, especially if it can be explained. And more on topic, a big +1 to this change. 

comment:15 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_work

The PDF documentation doesn't build:

! Missing { inserted.
<to be read again> 
                   $
l.15158 $_an_element_$ 
                       for the parent. But now, the following example works:
?
! Emergency stop.
<to be read again> 
                   $
l.15158 $_an_element_$ 
                       for the parent. But now, the following example works:
!  ==> Fatal error occurred, no output PDF file produced!

Changed 4 years ago by SimonKing

Combined patch

comment:16 Changed 4 years ago by SimonKing

  • Status changed from needs_work to positive_review

I hope double back ticks do the trick (I only changed single into double back tick, so, I hope I can directly revert to "positive_review")

Apply trac_14249-coercion_without_an_element.patch

comment:17 Changed 4 years ago by jdemeyer

  • Merged in set to sage-5.10.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
  • Summary changed from There should be no need to have _an_element_ implement to multiply elements to There should be no need to have _an_element_ to multiply elements
Note: See TracTickets for help on using tickets.