Possible coercion code bug, and coercion concerns
Carl Witty
method in NumberFieldElement_quadratic. This code had:
cdef Rational c = <Rational>_c
but in this case, _c was actually an Integer.
I'm not sure if _rmul_c_impl is actually allowed to assume that _c is
Rational. I think it may be, because the bad _rmul call is made by
LeftModuleAction.__init__, and I see a comment in that function,
# Objects are implemented with the assumption that
# _rmul_ is given an element of the basering
So it looks like LeftModuleAction is trying to make sure to only pass
a Rational to this function, but due to some bug is actually allowing
an Integer through. I haven't tried to understand this code in
LeftModuleAction, though, so I could be wrong.
So that's the possible bug. Now for the concerns:
1) Is there a document describing the new coercion model? If so,
where? If not, there really needs to be... it's too complicated to
understand by reading the code (I've tried more than once).
2) The automatic detection of possible actions scares me. It seems
fragile and overly magical. Would it be possible to disable this
(which would presumably slow things back down), then document a way
for classes to have fast coercions/actions (preferably in the
Programmer's Guide)? I think if there was a recommended, documented
way to have an explicit set of efficient coercions/actions, we could
get pretty much all of Sage optimized within a few releases.
Carl Witty
Robert Bradshaw
> William just reported #1044, which I tracked down to the _rmul_c_impl
> method in NumberFieldElement_quadratic. This code had:
> cdef Rational c = <Rational>_c
> but in this case, _c was actually an Integer.
>
> I'm not sure if _rmul_c_impl is actually allowed to assume that _c is
> Rational. I think it may be, because the bad _rmul call is made by
> LeftModuleAction.__init__, and I see a comment in that function,
> # Objects are implemented with the assumption that
> # _rmul_ is given an element of the basering
> So it looks like LeftModuleAction is trying to make sure to only pass
> a Rational to this function, but due to some bug is actually allowing
> an Integer through. I haven't tried to understand this code in
> LeftModuleAction, though, so I could be wrong.
I will look into this, but you are right that _rmul_c_impl requires
an element of the basering. This is a property of the old coercion
model, nothing new, and some rings assumed it already.
> So that's the possible bug. Now for the concerns:
>
> 1) Is there a document describing the new coercion model? If so,
> where? If not, there really needs to be... it's too complicated to
> understand by reading the code (I've tried more than once).
No, there is not, (at lest not all in one place) and what is
currently there is a mix of the old and new models which makes it
doubly bad. I am going to try and clean things up more while I change
everything to use the new cpdef functions.
> 2) The automatic detection of possible actions scares me. It seems
> fragile and overly magical. Would it be possible to disable this
> (which would presumably slow things back down), then document a way
> for classes to have fast coercions/actions (preferably in the
> Programmer's Guide)?
Right now, to define an action, one simply has to define any of
_rmul_, _lmul_, _r_action_, and _l_action_. The first two assume the
other element belongs to the basering. Is there a simpler way for the
_user_ to define an action?
> I think if there was a recommended, documented
> way to have an explicit set of efficient coercions/actions, we could
> get pretty much all of Sage optimized within a few releases.
That is optimistic, but perhaps possible. However, I would be wary of
such a project before we have a benchmarking system to make sure
we're actually fixing everything that we (massively) slow down.
- Robert
William Stein
> On Oct 31, 2007, at 8:40 PM, Carl Witty wrote:
>> I'm not sure if _rmul_c_impl is actually allowed to assume that _c is
>> Rational. I think it may be, because the bad _rmul call is made by
>> LeftModuleAction.__init__, and I see a comment in that function,
>> # Objects are implemented with the assumption that
>> # _rmul_ is given an element of the basering
>> So it looks like LeftModuleAction is trying to make sure to only pass
>> a Rational to this function, but due to some bug is actually allowing
>> an Integer through. I haven't tried to understand this code in
>> LeftModuleAction, though, so I could be wrong.
>
> I will look into this, but you are right that _rmul_c_impl requires
> an element of the basering. This is a property of the old coercion
> model, nothing new, and some rings assumed it already.
Yep. And in many cases ignoring that requirement will lead to a segfault.
It's the same as how _mul_c_impl, _add_c_impl, etc. all work. It was
the whole idea of the original "arithmetic architecture" stuff David
Harvey came up with at Sage Days 2, i.e., make it possible to code a fast
pathway under certain assumptions.
>> So that's the possible bug. Now for the concerns:
>>
>> 1) Is there a document describing the new coercion model? If so,
>> where? If not, there really needs to be... it's too complicated to
>> understand by reading the code (I've tried more than once).
>
> No, there is not, (at lest not all in one place) and what is
> currently there is a mix of the old and new models which makes it
> doubly bad. I am going to try and clean things up more while I change
> everything to use the new cpdef functions.
Those will likely help a lot with readability.
>> 2) The automatic detection of possible actions scares me. It seems
>> fragile and overly magical. Would it be possible to disable this
>> (which would presumably slow things back down), then document a way
>> for classes to have fast coercions/actions (preferably in the
>> Programmer's Guide)?
>
> Right now, to define an action, one simply has to define any of
> _rmul_, _lmul_, _r_action_, and _l_action_. The first two assume the
> other element belongs to the basering. Is there a simpler way for the
> _user_ to define an action?
I personally agree, that no matter what you'll have to define those functions,
so why not make them the only ones you have to define. It's very simple
and straightforward.
>> I think if there was a recommended, documented
>> way to have an explicit set of efficient coercions/actions, we could
>> get pretty much all of Sage optimized within a few releases.
>
> That is optimistic, but perhaps possible. However, I would be wary of
> such a project before we have a benchmarking system to make sure
> we're actually fixing everything that we (massively) slow down.
+1
-- William
Carl Witty
wrote:
> On Oct 31, 2007, at 8:40 PM, Carl Witty wrote:
> > 2) The automatic detection of possible actions scares me. It seems
> > fragile and overly magical. Would it be possible to disable this
> > (which would presumably slow things back down), then document a way
> > for classes to have fast coercions/actions (preferably in the
> > Programmer's Guide)?
>
> Right now, to define an action, one simply has to define any of
> _rmul_, _lmul_, _r_action_, and _l_action_. The first two assume the
> other element belongs to the basering. Is there a simpler way for the
> _user_ to define an action?
I think what I mean is that most of get_action_c_impl should be
removed, and that parents that want fast actions should be required to
set ._action_list (presumably by passing actions= to
Parent.__init__). In particular, comments like:
# TODO: if _xmul_/_x_action_ code does stuff like
# if self == 0:
# return self
# then _an_element_c() == 0 could be very bad.
worry me.
It seems like you're trying to guess things about how types are
implemented by probing them from outside, and that this is likely to
break for slightly-odd types.
Perhaps I wouldn't be worried if there were a clear, not-too-
complicated checklist for how to make sure a type works with the
coercion model. Things like "if _r_action accepts any value with
parent A, then it must accept all values with parent A" -- if that is
indeed a requirement of the coercion code.
Carl
Robert Bradshaw
> On Oct 31, 11:41 pm, Robert Bradshaw <rober...@math.washington.edu>
> wrote:
>> On Oct 31, 2007, at 8:40 PM, Carl Witty wrote:
>>> 2) The automatic detection of possible actions scares me. It seems
>>> fragile and overly magical. Would it be possible to disable this
>>> (which would presumably slow things back down), then document a way
>>> for classes to have fast coercions/actions (preferably in the
>>> Programmer's Guide)?
>>
>> Right now, to define an action, one simply has to define any of
>> _rmul_, _lmul_, _r_action_, and _l_action_. The first two assume the
>> other element belongs to the basering. Is there a simpler way for the
>> _user_ to define an action?
>
> I think what I mean is that most of get_action_c_impl should be
> removed, and that parents that want fast actions should be required to
> set ._action_list (presumably by passing actions= to
> Parent.__init__).
I originally envisioned passing everything up through the __init__
functions, but this is really messy as not all parents call their
super's __init__ method, and there is so much multiple-inheritance
among then that __init__ may get called multiple times too. Also, to
construct (say) an action, one needs pass in self, which (before
makes the call to Parent.__init__) may be incomplete.
> In particular, comments like:
> # TODO: if _xmul_/_x_action_ code does stuff like
> # if self == 0:
> # return self
> # then _an_element_c() == 0 could be very bad.
> worry me.
Yeah. Perhaps _an_element should be renamed to _nontrivial_element.
> It seems like you're trying to guess things about how types are
> implemented by probing them from outside, and that this is likely to
> break for slightly-odd types.
I want to make it as easy as possible for a user to provide right/
left multiplication, and the most natural thing to do seems to detect
whether or not such functions are implemented. Clear specification on
what _xmul_ should do and how it will be used should help greatly here.
> Perhaps I wouldn't be worried if there were a clear, not-too-
> complicated checklist for how to make sure a type works with the
> coercion model. Things like "if _r_action accepts any value with
> parent A, then it must accept all values with parent A" -- if that is
> indeed a requirement of the coercion code.
Point well taken, and I should be the one writing this all up (though
if anyone else wants to volunteer I'd be very glad).
- Robert