New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
There should be no need to have _an_element_ to multiply elements #14249
Comments
comment:1
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 So, that's an independent bug, which I fixed in #14264. |
Dependencies: #14264 |
Author: Simon King |
comment:2
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
Replying to @nbruin:
For general actions I agree. But:
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.
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 |
comment:4
That said, it is possible for a parent to return an action on request (implement And my patch does introduce such a |
comment:5
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
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
Hey Simon, Some minor things:
Thanks, Travis |
Fixes according to Travis' remarks |
comment:8
Attachment: trac14249-fix_doc.patch.gz I have added a new patch that hopefully succeeds in addressing Travis' comments. |
comment:9
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
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. |
Reviewer: Travis Scrimshaw |
comment:11
Replying to @simon-king-jena:
One workaround would be to fold the two patches into the first one, and leave the second one around for review. |
comment:12
OK, the patches got folded. Apply trac_14249-coercion_without_an_element.patch |
This comment has been minimized.
This comment has been minimized.
comment:13
Looks good to me. Thanks Simon. |
comment:14
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
The PDF documentation doesn't build:
|
Combined patch |
comment:16
Attachment: trac_14249-coercion_without_an_element.patch.gz 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 |
Merged: sage-5.10.rc0 |
We currently have
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 letan_element()
be called only if no element is passed.Apply
Depends on #14264
CC: @nthiery
Component: coercion
Author: Simon King
Reviewer: Travis Scrimshaw
Merged: sage-5.10.rc0
Issue created by migration from https://trac.sagemath.org/ticket/14249
The text was updated successfully, but these errors were encountered: