Add sub-pixel corner detection capability #682 #685

zygmuntszpak · 2017-12-01T13:02:14Z

Introduces the function imcorner_subpixel as well as several requisite
support functions to facilitate the ability to detect corners to
sub-pixel precision.

The imcorner_subpixel uses the existing imcorner functionality to obtain
initial corner response values and a binary corner indicator matrix. The
corner response values and the indicator matrix are used in a univariate
quadratic interpolation scheme to determine the corner coordinates to
sub-pixel precision.

The new imcorner_subpixel function returns a length-3 NTuple of type
HomogeneousPoint2D, which represents the homogeneous coordinates of a
planar point.

Evizero · 2017-12-01T13:06:18Z

src/Images.jl

@@ -18,6 +18,7 @@ using Base: depwarn
 using Base.Order: Ordering, ForwardOrdering, ReverseOrdering

 using Compat
+using StaticArrays


minor comment: this needs a corresponding entry in the REQUIRE file

Thanks, I will amend the commit and add the entry in the REQUIRE file. I will also add some more tests as per the codecov suggestions. ~~I'm not sure what to do about the failing tests on the win32 build. It seems to be a matter of precision because the approximately equal symbol is failing. Do you have any suggestions for that?~~

codecov · 2017-12-02T10:37:56Z

Codecov Report

Merging #685 into master will increase coverage by 0.4%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #685     +/-   ##
=========================================
+ Coverage   88.77%   89.18%   +0.4%     
=========================================
  Files          13       13             
  Lines        1167     1211     +44     
=========================================
+ Hits         1036     1080     +44     
  Misses        131      131

Impacted Files	Coverage Δ
src/corner.jl	`89.56% <100%> (+4.75%)`	⬆️
src/Images.jl	`91.66% <100%> (+16.66%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6f85f6...d611b92. Read the comment docs.

zygmuntszpak · 2017-12-02T12:24:17Z

Looking at the failed build for Win32 and the debug info I printed I no longer think it is failing because of precision. Could it be something more fundamental in Base? The stack trace seems to suggest there is something wrong with the statement all(ids[i].coords .≈ corner_pts[i].coords) in general, and not that the approximately true statement doesn't hold.

The problem was that the length of ids[i].coords does not match the length of corner_pts[i].coords. This is because the function corner_pts = imcorner_subpixel(img, Percentile(96), method = harris) returns a different number of corners on Win32 and Win64. It seems that the result of Percentile is slightly different on Win32 and Win64 when using a value of 96. I've set the value to 98 and on my home machine the Win32 and Win64 builds now both return the same number of points. Since I am relying on imcorner I expect that imcorner will also return different results for different percentiles on Win32 and Win64.

 Expression: all(ids[i].coords .≈ corner_pts[i].coords)
Stacktrace:
 [1] macro expansion at C:\Users\appveyor\.julia\v0.6\Images\test\corner.jl:110 [inlined]
 [2] macro expansion at .\test.jl:860 [inlined]
 [3] macro expansion at C:\Users\appveyor\.julia\v0.6\Images\test\corner.jl:33 [inlined]
 [4] macro expansion at .\test.jl:860 [inlined]
 [5] anonymous at .\<missing>:?

Evizero · 2017-12-03T09:51:53Z

src/Images.jl

+# divide each coordinate by the last coordinate everytime we attempt to index
+# into a matrix which could induce a performance hit. Note also that we return
+# a tuple of (y,x) since matrices are indexed according to row and then column.
+Base.to_indices(A::AbstractArray, p::Tuple{<: HomogeneousPoint2D}) = round(Int,  p[1].coords[2]), round(Int, p[1].coords[1])


Just to double check: p is limited to a 1-element tuple, but this is on purpose right?

It may be interesting to note (but no need to change it here) that Tuple get special treatment in Julia. For example they are a covariant type. This means that you could actually write p::Tuple{HomogeneousPoint2D} because for any T <: HomogeneousPoint2D it holds that Tuple{T} <: Tuple{HomogeneousPoint2D}

Yes, p is limited to a 1-element tuple. SimonDanish explained why in one of his replies to my question

I have quoted him below:

That's indeed confusing and is the aftermath of indexing a multi dimensional array with a single element. to_indices takes a tuple because it gets called called like this in the abstract array code:

#note, that indices isn't splatted, so it turns into a tuple:
getindex(A::AbstractArray, indices...) = getindex(A, to_indices(indices)...)
so calling getindex(A, Point(1.5, 1.5)) will call to_indices(A, (Point(1.5, 1.5),)).

Thanks for your remark about covariant types, I will do some more reading on the subject.

This is one of these painful decisions. From a performance standpoint I completely understand why you've done it, OTOH it breaks the core equivalence-class property of HomogeneousPoint. How bad is the hit if you add @assert p.coords[end] == 1? @assert statements get skipped if you start julia with julia -O3.

Evizero · 2017-12-03T09:59:59Z

src/corner.jl

+See [`corner2subpixel`](@ref) for more details of the interpolation scheme. 
+
+"""
+function imcorner_subpixel(img::AbstractArray; method::Function = harris, args...)


@timholy Do (or should) we have a convention for naming catch-all optional positional or named arguments?

Personally I adapted the convention of using args... for positional arguments and kw... for keyword arguments.

In this instance I just copied and pasted the current definition of imcorner, so whatever change you suggest I will then also apply to the existing imcorner code.

Images developed over a long enough period of time that there isn't as much consistency as would be desirable. I also tend to use args... for positional, and kw... or kwargs... for keywords.

Since I have followed the current convention of imcorner, how about we leave it as args for the purpose of this commit? We could raise a new issue for systematically checking the Image package for consistent usage of args and kw? I just noticed that there appear to be several places where kwargs should have been used, such as gradcovs.

In the meantime, I will address the other issues that Evizero has pointed out and amend the current commit.

Evizero · 2017-12-03T10:02:18Z

src/corner.jl

+    nrows,ncolumns = size(corner_indicator)
+    row, col, _ = findnz(corner_indicator)
+    ncorners = length(row)
+    corners = fill(Images.HomogeneousPoint2D((0.0,0.0,0.0)),ncorners)


Let's remove the Images. prefix here, since we are within the Images module

Thanks for spotting that, I thought I had systematically removed any reference to Images.

Evizero · 2017-12-03T10:03:45Z

src/corner.jl

+```
+Refines integer corner coordinates to sub-pixel precision.
+
+The function takes as input a matrix reresenting corner responses


typo representing

Evizero · 2017-12-03T10:07:50Z

src/corner.jl

+    \end{bmatrix}
+```
+the coefficients of the quadratic polynomial can be found by solving the
+system of equations `b = Ax`. The result is given by `x = inv(A)b`.


Since you used the math environment in the previous paragraph I think its worth considering doing it here for b = Ax and below as well (the two backticks environment instead of one)

Sure, no worries. Do you know how I might visualise what the LaTeX markup will look like? My version of Juno does not support Latex preview, and I didn't see any such functionality in Visual Studio Code. I'm not sure how to build the documentation so that I can actually see the LaTeX markup.

Do you have access to Jupyter? I think it does the appropriate rendering for docstrings

Evizero · 2017-12-03T10:09:29Z

test/corner.jl

@@ -29,6 +29,118 @@ using Base.Test, Images, Colors, FixedPointNumbers
    	for id in ids expected_corners[id] = 0 end
    	@test sum(corners .!= expected_corners) < 3
    end
+
+	@testset "Corners Sub-pixel API" begin


These are tabs I think. Could you change this to use spaces instead?

How can you tell that they are tabs not spaces? I was using tab, but I thought that the editors (VS Code and Juno) were set up to automatically convert tabs to spaces.

Just by selecting the text with the mouse basically. You'll see that you can just select one giant white space. In contrast if using spaces it would select a bunch of small whitespaces

Evizero · 2017-12-03T10:11:37Z

This looks like a nicely made addition. I left a couple of minor and style related comments. I am afraid I don't know much about the subject itself. Maybe someone else here could review the functionality.

timholy · 2017-12-03T11:03:55Z

I'll review this, but first I'd really like to thank @Evizero. In recent months my administrative responsibilities at work have increased enormously, to the point where it's hard for me to put much time into development. Having help with some of the review/bugfix/merge responsibilities will allow me to focus on higher-level issues, and collectively we'll ensure that JuliaImages keeps thriving!

Evizero · 2017-12-03T11:10:01Z

happy to help where my skillset allows it. You were a great tutor to me when I started working with Images.jl, so I'd like to give back how I can (though my knowledge about image processing is very spotty)

timholy

Love it, this is great! Thanks for being so conscientious about the docstrings and tests.

timholy · 2017-12-03T11:13:10Z

src/Images.jl

@@ -64,6 +66,33 @@ Indicate that `x` should be interpreted as a [percentile](https://en.wikipedia.o
 """
 struct Percentile{T} <: Real p::T end

+
+"""
+HomogeneousPoint2D(x::NTuple{3, T})


Would it be worth calling this

struct HomogeneousPoint{T<:AbstractFloat,N} coords::NTuple{N,T} end

The concept is a general one, even if its first application (imcorner) is currently limited to 2d.

timholy · 2017-12-03T11:17:57Z

src/Images.jl

+# divide each coordinate by the last coordinate everytime we attempt to index
+# into a matrix which could induce a performance hit. Note also that we return
+# a tuple of (y,x) since matrices are indexed according to row and then column.
+Base.to_indices(A::AbstractArray, p::Tuple{<: HomogeneousPoint2D}) = round(Int,  p[1].coords[2]), round(Int, p[1].coords[1])


This is one of these painful decisions. From a performance standpoint I completely understand why you've done it, OTOH it breaks the core equivalence-class property of HomogeneousPoint. How bad is the hit if you add @assert p.coords[end] == 1? @assert statements get skipped if you start julia with julia -O3.

timholy · 2017-12-03T11:21:06Z

src/corner.jl

+"""
+```
+corners = corner2subpixel(responses::Matrix{Float64},corner_indicator::Array{Bool,2})
+        -> Vector{HomogeneousPoint2D{3,Float64}}


I see you considered having an extra parameter to HomogeneousPoint already 🙂. Whatever gets finally decided, make sure this docstring is in sync.

timholy · 2017-12-03T11:21:54Z

src/corner.jl

+The function takes as input a matrix reresenting corner responses
+and a boolean indicator matrix denoting the integer coordinates of a corner
+in the image. The output is a vector of type `HomogeneousPoint2D`
+(an `NTuple{3, Float64}`) storing the sub-pixel coordinates of the corners.


change to [`HomogeneousPoint`](@ref) and drop the statement in parentheses

timholy · 2017-12-03T11:31:44Z

src/corner.jl

+    invA = @SMatrix [0.5 -1.0 0.5; -0.5 0.0 0.5; 0.0 1.0 -0.0]
+    for k = 1:ncorners
+        # Corners on the perimeter of the image will not be interpolated.
+        if  row[k] == 1 || row[k] == nrows || col[k] == 1 || col[k] == ncolumns


imcorner works even for non-1 arrays:

img = testimage("cam") imgo = OffsetArray(img, (-2, 5)) imcorner(imgo)

It would be better to compare to the first/last values of indices(corner_indicator).

timholy · 2017-12-03T11:33:06Z

src/corner.jl

+    Corners on the boundary of the image are not refined to sub-pixel precision.
+
+"""
+function corner2subpixel(responses::Matrix{Float64}, corner_indicator::Array{Bool,2})


I would favor more general choices like corner_indicator::AbstractMatrix{Bool}. See below (re indices) to see how other types might arise.

timholy · 2017-12-03T11:36:37Z

src/corner.jl

+
+"""
+function unsafe_neighbourhood_4(matrix::Matrix{Float64},r::Int,c::Int)
+    @views center = matrix[r,c]


I don't think you need the @views here, you're just extracting a single value from the array.

In light of your comment regarding OffsetArray, I presume I have to change matrix::Matrix{Float64} to matrix::Union{OffsetArrays.OffsetArray{Float64,2,Array{Float64,2}}, Matrix{Float64}}?

Similarly, responses::Matrix{Float64} would need to become responses::Union{OffsetArrays.OffsetArray{Float64,2,Array{Float64,2}}, Matrix{Float64}}?

Why put a type on at all? (There is no performance advantage to specifying the type.) The only thing that adding the type does here is ensure that a MethodError gets thrown by an inappropriate call to this function, rather than some error thrown by, e.g., the indexing operations inside the body of this function.

It wouldn't be crazy to restrict it to two-dimensional AbstractArrays, though:

unsafe_neighbourhood_4(matrix::AbstractMatrix, r::Int, c::Int)

timholy · 2017-12-03T11:41:18Z

test/corner.jl

+
+		ids = map(HomogeneousPoint2D,
+                    [(4.244432486132958, 4.244432486132958, 1.0),
+					 (4.244432486132958, 16.755567513867042, 1.0),


indentation issues

timholy · 2017-12-03T11:42:37Z

test/corner.jl

@@ -188,4 +300,4 @@ using Base.Test, Images, Colors, FixedPointNumbers

 end

-nothing
+nothing


Add blank line at end

zygmuntszpak · 2017-12-07T05:08:50Z

The latest pull-request addresses all the comments and suggestions. If I have left anything out I did so unintentionally.

zygmuntszpak · 2017-12-09T23:44:54Z

I fixed a typo I spotted in one if the docstrings,

timholy

Just the docstring change and this is ready to go.

timholy · 2017-12-10T12:19:28Z

src/Images.jl

+# the corresponding Cartesian coordinates. 
+Base.to_indices(A::AbstractArray, p::Tuple{<: HomogeneousPoint}) = homogeneous_point_to_indices(A, p)
+
+function homogeneous_point_to_indices(A::AbstractArray, p::Tuple{<: HomogeneousPoint})


This might be prettier if you extract the HomogeneousPoint in to_indices and just pass that:

Base.to_indices(A::AbstractArray, p::Tuple{<: HomogeneousPoint}) = homogeneous_point_to_indices(p[1])

because then you don't have to write p[1] everywhere in homogeneous_point_to_indices.

A more typical "Julian" style would be to avoid the branch and provide two methods:

function homogeneous_point_to_indices(p::HomogeneousPoint{T,3}) where T if p.coords[end] == 1 ... end function homogeneous_point_to_indices(p::HomogeneousPoint) if p.coords[end] == 1 ... end

However, the compiler should be smart enough that it will evaluate your if (length(p[1]).coords) == 3 at compile time so it avoids introducing another branch (in other words, in terms of generated code yours will be the same as mine).

All these are optional, just wanted you to know.

timholy · 2017-12-10T12:23:39Z

src/corner.jl

+"""
+```
+corners = corner2subpixel(responses::AbstractMatrix,corner_indicator::AbstractMatrix{Bool})
+        -> Vector{HomogeneousPoint{3,Float64}}


Parameters are swapped (should be Float64,3).

Introduces the function imcorner_subpixel as well as several requisite support functions to facilitate the ability to detect corners to sub-pixel precision. The imcorner_subpixel uses the existing imcorner functionality to obtain initial corner response values and a binary corner indicator matrix. The corner response values and the indicator matrix are used in a univariate quadratic interpolation scheme to determine the corner coordinates to sub-pixel precision. The new imcorner_subpixel function returns a length-3 NTuple of type HomogeneousPoint, which represents the homogeneous coordinates of a planar point.

zygmuntszpak · 2017-12-11T11:02:22Z

Done. Thank you for all the excellent advice.

timholy · 2017-12-17T11:10:42Z

Sorry I forgot to hit merge! Thanks again!

zygmuntszpak · 2017-12-17T12:45:50Z

@timholy If I want this to appear when I do Pkg.update("Images") do I have to wait for you to tag a release? Do you have a policy for tagging releases?

timholy · 2017-12-17T13:02:46Z

Just tagged (will have to wait for the METADATA.jl PR to be merged). Thanks for the reminder!

Evizero reviewed Dec 1, 2017

View reviewed changes

zygmuntszpak force-pushed the imcorner_subpixel branch from e4f62c6 to a5ad5d9 Compare December 2, 2017 10:37

zygmuntszpak force-pushed the imcorner_subpixel branch from a5ad5d9 to 72f7db2 Compare December 3, 2017 04:12

Evizero reviewed Dec 3, 2017

View reviewed changes

timholy reviewed Dec 3, 2017

View reviewed changes

zygmuntszpak force-pushed the imcorner_subpixel branch from 72f7db2 to 66fcf68 Compare December 7, 2017 03:32

zygmuntszpak force-pushed the imcorner_subpixel branch from 66fcf68 to e254a97 Compare December 9, 2017 23:19

timholy reviewed Dec 10, 2017

View reviewed changes

zygmuntszpak force-pushed the imcorner_subpixel branch from e254a97 to d611b92 Compare December 11, 2017 09:56

timholy merged commit 18590c7 into JuliaImages:master Dec 17, 2017

zygmuntszpak deleted the imcorner_subpixel branch December 17, 2017 11:44

@@ @@ -188,4 +300,4 @@ using Base.Test, Images, Colors, FixedPointNumbers @@
               end
-              nothing
+              nothing

Add sub-pixel corner detection capability #682 #685

Add sub-pixel corner detection capability #682 #685

Conversation

zygmuntszpak commented Dec 1, 2017

Choose a reason for hiding this comment

zygmuntszpak Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 2, 2017 • edited Loading

Codecov Report

zygmuntszpak commented Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Evizero commented Dec 3, 2017

timholy commented Dec 3, 2017

Evizero commented Dec 3, 2017

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygmuntszpak commented Dec 7, 2017

zygmuntszpak commented Dec 9, 2017

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygmuntszpak commented Dec 11, 2017

timholy commented Dec 17, 2017

zygmuntszpak commented Dec 17, 2017

timholy commented Dec 17, 2017

zygmuntszpak Dec 2, 2017 •

edited

Loading

codecov bot commented Dec 2, 2017 •

edited

Loading

zygmuntszpak commented Dec 2, 2017 •

edited

Loading