New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FAST, oFAST and rBRIEF #699
Conversation
@ahojnnes : Please review |
|
||
|
||
def corner_fast(image, n=12, threshold=0.15): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
@ankit-maverick I really like your corner_fast implementation! I have not yet had time to review your orb function. Two more notes to corner_fast. Please move the Python implementation to the existing corner.py and corner_cy.pyx files. Have you done any comparisons to the OpenCV FAST results? Are they correct? |
if circle_intensity > image[i, j] + threshold: | ||
# Brighter pixel | ||
bins[k] = 'b' | ||
elif circle_intensity < image[i, j] - threshold: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could save the value of image[i, j] in an intermediate variable.
@ahojnnes : I will do the comparison with OpenCV FAST and let you know the results. Before pushing, I verified it by picking 5-6 random corners detected by The |
Remove image wrapper.
cdef int[:] CP = (np.round(3 * np.cos(2 * np.pi * np.arange(16, dtype=np.double) / 16))).astype(np.int32) | ||
|
||
|
||
cdef inline _get_corner_response(double[:, ::1] image, int i, int j, char[:] bins, char check_state, int n, double threshold, double[:, ::1] corner_response): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Py_ssize_t for i, j, n
return response value, so you can avoid the corner_response parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function should get a different name, which associates it with "fast"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahojnnes : I considered avoiding the corner_response parameter, but realized that we would have to assign zero to all the non-corners using return 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's wrong with returning 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing wrong with it; just that I was thinking of some micro-optimization.
Computing the angle of the keypoints should be a easy one now (I would implement it in Cython as well): for n in range(keypoints.shape[0]): Make sure to define the moments functions as cpdef so that they can be called efficiently from Cython, which also requires a pxd (header) file (have a look at the _shared package). |
|
||
image = img_as_float(image) | ||
image = np.ascontiguousarray(image) | ||
corner_response = _corner_fast(image, n, threshold) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with the other corner detectors we should return the raw corner response. Non-maximal suppression is implemented in corner_peaks.
speed_sum_d = 0 | ||
|
||
for k in range(16): | ||
circle_intensity = image[i + RP[k], j + CP[k]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to save the intensity on the circle in an intermediate array double[16], so that you can reuse the values in _get_fast_corner_response, also we should consider to save lower_threshold = current_pixel - threshold
and upper_threshold
.
@ankit-maverick Are you still working on this one? |
@Ajohannes : I have some deadlines till Saturday. I will push on Sunday. On Fri, Oct 25, 2013 at 6:10 PM, Johannes Schönberger <
|
Great, ping when you’re ready. |
@ahojnnes : Can you review the above changes? I will change the examples/tests/docs as soon as we are finalized on the implementation. |
return kpts_recarray | ||
else: | ||
best_indices = harris_measure.argsort()[::-1][:n_keypoints] | ||
return kpts_recarray[best_indices] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also do kpts_recarray.sort(index=...)[:n_keypoints]
@ankit-maverick Looks good to me. You could go ahead with the rest. |
@ahojnnes : I think 'x' and 'y' is a better choice instead of 'row' and 'col'. Let me know your opinion. |
Hm, I always prefer row and col (r and c), because it makes crystal clear to which dimension of the array it belongs - whereas x and y has lead to several confusions in the past, also here at scikit-image… But we could use both. x and y as an alias for row and col? What do you think? |
@ahojnnes : Ok. I get your point now. |
Great, let me know if you need any further help! |
@ahojnnes , I feel that |
Good point! +1 for making it public. |
Hi @ahojnnes , apologies for stretching this so long. Can you review the minor changes in the previous 2-3 commits? I will update the doc/examples with the recarray changes now. Let me know if I am missing anything else. |
Hi @ahojnnes , the matching performance seems to have reduced a bit after introducing the recarray changes. I guess it has something to do with |
This looks good to me so far, but I have some more comments. I guess it saves time both for you and me if I do the changes directly in the code and send a PR against your branch and you can review them. What do you think? |
@ahojnnes : Sure, I agree. I have my end-terms till the next Thursday, and then I can work on this and the FREAK descriptor. And woah!! This is the first time I am seeing your face!! |
@ahojnnes : My end-terms are over and I am free. Can we decide how we should proceed on finishing this? In a few days or so, I am also planning to work on blob detection functions(https://en.wikipedia.org/wiki/Blob_detection). |
I’ll work on a PR this weekend and ping you here. |
@ankit-maverick I guess this PR can be closed then? Everything was contained in the ORB PR? |
@ahojnnes : Ok. Closing this one :) |
TO DO :
Replace the variables
keypoints
,scales
,orientations
,response
withkpts_recarray
: