Skip to content
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

Refac matching binary ctypes #202

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

amiotc
Copy link

@amiotc amiotc commented Mar 13, 2019

This PR follows the same logic as PR #193. It modifies the keypoints_match implementation in sift.py module, while preserving its API.

SIFT keypoints are now read from the txt file generated at the previous step, and stored into numpy arrays. These arrays are passed to keypoints_match_from_nparray, which calls the C++ code performing matches between two sets of SIFT keypoints.

The actual matching C code has been rewritten in the sift4ctypes.cpp file. The former matching implementation was using multiple C structures, which would have implied to format all the numpy arrays into these structures, and extract the returned structure into a numpy array. Also, most of the information stored in these structures was useless for the matching algorithm. Hence, rewriting the functions so that they accept arrays instead of the defined structures seemed the best option.

The called library returns an array, which is transformed in a numpy array and written in a text file.

carlodef
carlodef previously approved these changes Mar 26, 2019
@carlodef carlodef dismissed their stale review March 26, 2019 23:02

The tests fail

@carlodef
Copy link
Contributor

carlodef commented Apr 8, 2019

Thank you @amiotc. I've fixed a coordinates swap in the file sift4ctypes.cpp and changed the datatype of a ctypes.POINTER to ndpointer in sift.py. Could you check that everything's ok?

@amiotc
Copy link
Author

amiotc commented Apr 9, 2019

@carlodef I have checked your modifications. Everything seems fine for me. Thank you for the bugfix.

@carlodef
Copy link
Contributor

carlodef commented Apr 9, 2019

@amiotc Great, thank you! So now if @jmichel-otb and @dyoussef approve the pull request I think that we can merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants