new coercion model and actions
William Stein
As described in
http://trac.sagemath.org/sage_trac/ticaket/1044
in sage-2.8.10 this leads to a segfault:
sage: K.<a> = NumberField(x^2 - 5)
sage: B = K.maximal_order().basis();
sage: -1*B[1]
BOOM!
As does
sage: B[1]*-1
By putting some code to rain an exception in the number_field_quadratic_element.pyx
file I was able to track this down to a call made by the init method of
cdef class LeftModuleAction(Action):
in coerce.pyx. In particular, you have this code:
# TODO: detect this better
# if this is bad it will raise a type error in the subsequent lines, which we propagate
cdef RingElement g = G._an_element()
cdef ModuleElement a = S._an_element()
res = self._call_c(g, a)
And _call_c is:
cdef Element _call_c_impl(self, Element g, Element a):
if self.connecting is not None:
g = self.connecting._call_c(g)
if self.extended_base is not None:
a = self.extended_base(a)
return _rmul_c(<ModuleElement>a, <RingElement>g) # a * g
That line involving _rmul_c seems to me to be just totally wrong. The problem is
that _rmul_c *assumes* that g is an element of the base ring of the
parent of a. However, I think you're calling it as some sort of test and
hoping that a TypeError will be raised. Unfortunately, we get a
segmentation fault.
So could you clarify what you think _rmul_c does compared to what I
thought it was supposed to do when I wrote it? I only thought
of external code calling _rmultiply_by_scalar. Indeed, in the above
case if you replace the _rmul_c line by
return (<ModuleElement>a)._rmultiply_by_scalar(<RingElement>g)
then this seems to fix all the problems (but is maybe slower).
Note also that there are similar issues with _lmul_c in coerce.pyx, and
*shudder* with _ilmul_c, where you didn't implement an analogue of
_rmultiply_by_scalar yet.
The easy fixes I can think of are:
1. Rewrite _call_c_impl to call safe methods, i.e., _rmultiply_by_scalar,
_lmultiply_by_scalar, and _ilmultiply_by_scalar (which will have to be
written). What are the performance impacts?
-- OR --
2. Change the semantics of _rmul_c, _lmul_c, and _ilmul_c, etc. so that
they must raise a TypeError if the type of the scalar isn't in the base
ring. This will slow them down, of course, but I think it's what you
assumed already in writing coerce.pyx, perhaps all over the place.
If this is the case, it means
(a) looking at the code for probably about 30-40 functions, and
in many of those cases making changes just like the one carl
witty attached to trac #1044.
(b) documenting that _rmul_c_impl, _lmul_c_impl, etc., must raise
a TypeError if the types aren't right (i.e, they can't segfault).
I think 2 might be more in line with what you meant when writing coerce.pyx.
However it goes completely antithetical to how _mul_c_impl, _add_c_impl,
etc., are defined -- they are supposed to get to assume that the types
are right, so _rmul_c_impl should also have that luxury. Thus it makes
the most sense to do 1. But what impact will that have on coerce.pyx?
For example, _call_c_impl gets called every single time I write
-1*B[1]
-- William
--
William Stein
Associate Professor of Mathematics
University of Washington
http://wstein.org
Robert Bradshaw
_rmul_, _irmul_
_lmul_, _ilmul_
_rmultiply_by_scalar_
_lmultiply_by_scalar_
_r_action_
_l_action_
This seems to be redundant. What I am envisioning is:
_rmul_, _lmul_, _irmul_, _ilmul_ MUST be called with an element of
the basering, in the spirit of _add_, _mul_, etc. In practice these
are called by LeftModuleAction and RightModuleAction and return an
element with the same parent as self. These are optimized for speed.
_r_action_ and _l_action_ are for implementing left/right
multiplications of elements of elements not in the basering, and get
called by LAction and RAction.
In adition, if R is the basering of M, and there is a coercion S ->
R, then we call m._rmul_(R(s)) for s \in S. Also, if R does not act
of M, but we can extend M -> N such that R acts on N, we call N
(m)._rmul_(r).
I don't think we need a _rmultiply_by_scalar_ and
_lmultiply_by_scalar_. It's just yet another multiply function to try
and understand/implement.
- Robert
By the time it gets to line 785 of LeftModuleAction, G should be the
basering of S. Somewhere in the logic above I must have messed up.
The argument to _rmul_ MUST be an element of the basering. I think it
is even worth adding an assertion there to make sure.
William Stein
> Right now we have (not counting the _c and _impl variants)
>
> _rmul_, _irmul_
> _lmul_, _ilmul_
> _rmultiply_by_scalar_
> _lmultiply_by_scalar_
> _r_action_
> _l_action_
>
> This seems to be redundant. What I am envisioning is:
>
> _rmul_, _lmul_, _irmul_, _ilmul_ MUST be called with an element of
> the basering, in the spirit of _add_, _mul_, etc. In practice these
> are called by LeftModuleAction and RightModuleAction and return an
> element with the same parent as self. These are optimized for speed.
>
> _r_action_ and _l_action_ are for implementing left/right
> multiplications of elements of elements not in the basering, and get
> called by LAction and RAction.
>
> In adition, if R is the basering of M, and there is a coercion S ->
> R, then we call m._rmul_(R(s)) for s \in S. Also, if R does not act
> of M, but we can extend M -> N such that R acts on N, we call N
> (m)._rmul_(r).
>
> I don't think we need a _rmultiply_by_scalar_ and
> _lmultiply_by_scalar_. It's just yet another multiply function to try
> and understand/implement.
Basically, I completely agree. More below...
_rmultiply_by_scalar and _lmultiply_by_scalar were the functions
that implemented the old coercion model, just like __mul__ implements
it. I.e., _rmultiply_by_scalar decides whether coercisons are needed,
etc., and if so does them, then calls _rmul_. You have to
either keep those functions *and* call them where you are calling
_rmul_c right now, or you have to rewrite the coerce.pyx code
to correctly use _rmul_c, according to the rules you lay out above.
I would be happied with you doing exactly what you propose above, i.e.,
making sure coerce.pyx always uses _rmul_c, etc., correctly, and getting
rid _rmultiply_by_scalar (which should be redundant).
William
Robert Bradshaw
Sounds like a good plan. I'll get on it soon.
- Robert