code review 5395042: graphics/edge: add canny edge detection

graphics/edge: add canny edge detection

[crawshaw1, 2011-11-17 15:15:11]

Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://crawshaw%40google.com@code.google.com/p/graphics-go/

[crawshaw1, 2011-11-17 15:17:20]

Two questions in here. First, should a subpackage have its own testdata dir?

Second, I have about 100 lines of test code copied between the
subpackage and its parent, should I create some new testing subpackage
and put it in there?

On Thu, Nov 17, 2011 at 7:15 AM,  <crawshaw@google.com> wrote:
> Reviewers: nigeltao,
>
> Message:
> Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://crawshaw%40google.com@code.google.com/p/graphics-go/
>
>
> Description:
> graphics/edge: add canny edge detection
>
> Please review this at http://codereview.appspot.com/5395042/
>
> Affected files:
>  A graphics/edge/Makefile
>  A graphics/edge/canny.go
>  A graphics/edge/canny_test.go
>  A graphics/edge/gauss.go
>  A graphics/edge/gauss_test.go
>  A graphics/edge/sobel.go
>  A graphics/edge/sobel_test.go
>  A testdata/gopher-dog.png
>  A testdata/gopher-log.png
>  A testdata/prewitt-dir.png
>  A testdata/prewitt-mag.png
>  A testdata/scharr-dir.png
>  A testdata/scharr-mag.png
>  A testdata/sobel-dir.png
>  A testdata/sobel-mag.png
>
>
>

[nigeltao, 2011-11-20 11:23:36]

I'm OK with a testdata/edge directory, and a graphics/graphicstest package.

http://codereview.appspot.com/5395042/diff/11002/graphics/edge/gauss.go
File graphics/edge/gauss.go (right):

http://codereview.appspot.com/5395042/diff/11002/graphics/edge/gauss.go#newco...
graphics/edge/gauss.go:14: // LaplacianOfGaussian approximates a 2D laplacian of
gaussian with a convolution kernel.
80 chars for doc comments.

"laplacian" and "gaussian" should also be capitalized.

http://codereview.appspot.com/5395042/diff/11002/graphics/edge/sobel.go
File graphics/edge/sobel.go (right):

http://codereview.appspot.com/5395042/diff/11002/graphics/edge/sobel.go#newco...
graphics/edge/sobel.go:77: angle := math.Atan(cy / cx)
What happens if cx == 0?

Also, rather than performing a relatively expensive Atan calculation in the
inner loop, can you instead compare cy/cx to ±(math.Sqrt2 - 1), etc?

http://codereview.appspot.com/5395042/diff/11002/graphics/edge/sobel.go#newco...
graphics/edge/sobel.go:100: // dir pixels hold the rounded direction value
either 0, 45, 90, or 135.
The docs should say whether 45 means NorthWest/SouthEast or NorthEast/SouthWest.
I'd expect the former, consistent with Rotate(90) meaning clockwise, but IIUC
you've implemented the latter.

http://codereview.appspot.com/5395042/diff/11002/graphics/edge/sobel.go#newco...
graphics/edge/sobel.go:101: func Sobel(mag, dir *image.Gray, src image.Image)
error {
"magDst" and "dirDst" might be clearer parameter names.