Review: Canny

[Dan Farmer]

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
>>
>