Review: Canny

[Thouis (Ray) Jones]

Looks good to me.

2011/4/2 Dan Farmer <dfarmernv at gmail.com>:
> I've pushed the suggested changes :
> https://github.com/dfarmer/scikits.image/compare/master...dfarmer-filters-canny
>
> Thanks,
> Dan
>
> 2011/3/31 Dan Farmer <dfarmernv at gmail.com>:
>> I will try to address your comments tonight and also see about
>> removing smooth.py and just incorporating the one function we're using
>> into canny.py for now as Thouis suggested. Thanks to both of you for
>> looking at it. I'll message back when I've pushed the changes.
>>
>> -Dan
>>
>> 2011/3/31 Stéfan van der Walt <stefan at sun.ac.za>:
>>> Hi Dan
>>>
>>> On Thu, Mar 31, 2011 at 1:02 AM, Dan Farmer <dfarmernv at gmail.com> wrote:
>>>> https://github.com/dfarmer/scikits.image/compare/master...dfarmer-filters-canny
>>>
>>> I read through your patch and made some preliminary comments.
>>>
>>>> Mostly just trying to follow procedure. I already mentioned my
>>>> concerns in the previous thread. I made one stab at introduced a
>>>> "None" default for the mask, but I got hung up and reverted it. The
>>>> default I was going to propose was np.ones(img.shape,bool) (and after
>>>> the fact I even noticed that's how it is used in one of the unit
>>>> tests). But I started thinking that that could be quite wasteful of
>>>> memory if you were working with large images (on my test use case with
>>>> ~512x512 images it's about 300 KB for the "fake" mask).
>>>
>>> It seems as though this specific implementation of the algorithms
>>> relies on creating the mask, so I don't think you can get away from
>>> it.  The typical way to do it would be:
>>>
>>> def canny(..., mask=None, ...):
>>>    if mask is None:
>>>        mask = np.ones(x.shape, dtype=bool)
>>>
>>>> The problem I had was that if I don't allocate the emask array I get
>>>> run-time errors starting at line 129 (in the diff of canny.py) because
>>>> the arrays all have different lengths if they aren't logical_and'd
>>>> with emask above.
>>>
>>> Yes, I think the only way to avoid allocating the mask explicitly is
>>> to rewrite the algorithm in Cython, where you can modify behaviour
>>> inside the for-loop.
>>>
>>> Regards
>>> Stéfan
>>>
>>
>