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@sun.ac.za>:
Hi Dan
On Thu, Mar 31, 2011 at 1:02 AM, Dan Farmer <dfarmernv@gmail.com> wrote:
https://github.com/dfarmer/scikits.image/compare/master...dfarmer-filters-ca...
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