[Cython] Fwd: Re: [cython-users] checking for "None" in nogil function

[Dag Sverre Seljebotn]

On 05/07/2012 03:04 PM, Stefan Behnel wrote:
> Dag Sverre Seljebotn, 07.05.2012 13:48:
>> Here you go:
>>
>> def foo(np.ndarray[double] a, np.ndarray[double] out=None):
>>      if out is None:
>>          out = np.empty_like(a)
>
> Ah, right - output arguments. Hadn't thought of those.
>
> Still, since you pass None explicitly as a default argument, this code
> wouldn't be impacted by disallowing None for buffers by default. That case
> is already handled specially in the compiler. But a better default would
> prevent the *first* argument from being None.
>
> So, basically, it would do the right thing straight away in your case and
> generate safer and more efficient code for it, whereas now you have to test
> 'a' for being None explicitly and Cython won't understand that hint due to
> insufficient static analysis. At least, since my last commit you can make
> Cython do the same thing by declaring it "not None".

Yes, thanks!

>>>> It's really no different from cdef classes.
>>>
>>> I find it at least a bit more surprising because a buffer unpacking
>>> argument is a rather strong hint that you expect something that supports
>>> this protocol. The fact that you type your function argument with it hints
>>> at the intention to properly unpack it on entry. I'm sure there are lots of
>>> users who were or will be surprised when they realise that that doesn't
>>> exclude None values.
>>
>> Whereas I think there would be more users surprised by the opposite.
>
> We've had enough complaints from users about None being allowed for typed
> arguments already to consider it at least a gotcha of the language.
>
> The main reason we didn't change this behaviour back then was that it would
> clearly break user code and we thought we could do without that. That's
> different from considering it "right" and "good".
>
>
>>>>> And I remember that we wanted to change the default settings for extension
>>>>> type arguments from "or None" to "not None" years ago but never actually
>>>>> did it.
>>>>
>>>> I remember that there was such a debate, but I certainly don't remember
>>>> that this was the conclusion :-)
>>>
>>> Maybe not, yes.
>>>
>>>
>>>> I didn't agree with that view then and
>>>> I don't now. I don't remember what Robert's view was...
>>>>
>>>> As far as I can remember (which might be biased towards my personal
>>>> view), the conclusion was that we left the current semantics in place,
>>>> relying on better control flow analysis to make None-checks cheaper, and
>>>> when those are cheap enough, make the nonecheck directive default to
>>>> True
>>>
>>> At least for buffer arguments, it silently corrupts data or segfaults in
>>> the current state of affairs, as you pointed out. Not exactly ideal.
>>
>> No different than writing to a field in a cdef class...
>
> Hmm, aren't those None checked? At least cdef method calls are AFAIR.

Not at all. That's my whole point -- currently, the rule for None in 
Cython is "it's your responsibility to never do a native operation on None".

I don't like that either, but that's just inherited from Pyrex (and many 
projects would get speed regressions etc.).

I'm not against changing that to "we safely None-check", if done nicely 
-- it's just that that should be done everywhere at once.

In current master (and as far back as I can remember), this code:

cdef class A:
     cdef int field
     cdef int method(self):
         print self.field
def f():
     cdef A a = None
     a.field = 3
     a.method()

Turns into:

   __pyx_v_a = ((struct __pyx_obj_5test2_A *)Py_None);
   __pyx_v_a->field = 3;
   ((struct __pyx_vtabstruct_5test2_A *) 
__pyx_v_a->__pyx_vtab)->method(__pyx_v_a);



> I think we should really get back to the habit of making code safe first
> and fast afterwards.

Nobody has argued otherwise for some time (since the cdivision thread I 
believe), this is all about Pyrex legacy. Guess part of the story is 
that there's lots of performance-sensitive code in SAGE using cdef 
classes which was written in Pyrex before Cython was around...

In fact, the nonecheck directive was written by yours truly! And I 
argued for making it the default at the time!

> Because it uses syntax that is expected to unpack the buffer. If that
> buffer doesn't exist, I'd expect an error. It's like using interfaces: I
> want something here that implements the buffer interface. If it doesn't -
> reject it.
>
> Besides, I hope you are aware that your argumentation stands on the (IMHO
> questionable) fact that "np.ndarray" by itself can be None by default. If
> np.ndarray should not be be allowed to be None by default, why should
> np.ndarray[double]? That argument works in both ways.

I'm well aware of it...

Dag