Disclaimer: The code below is (partly) untested, so there can be some rough edges that need polishing.
Style
First, some style-related notes. Python has an official Style Guide, often just called PEP8. The part most relevant to your code IMHO is documentation strings. In essence, you should follow the official recommendation to put your function documentation in """triple quotes"""
inside the function body. An example adapted from your code:
def compare_images(key_point_image1, descriptor_image1,
key_point_image2, descriptor_image2):
"""
uses Euclidean distance between common key points to judge similarity
in FLANN (Fast Library for Approximate Neighbors) matching
"""
...
You often use an explicit list to return multiple values from a function. This is unusual and likely unnecessary. Most often there is no explicit type when returning multiple values, e.g.
def swap(a, b):
return b, a
Under the hood, Python returns a tuple from the function as can be seen if looking at type(swap(1, 2))
which will happily print tuple
in an interactive Python interpreter.
Avoid global variables
You should try avoid global variables whenever possible. And in your case the global variable is absolutely not necessary. We will cover that in a second.
read_data
read_data
is written wasteful. You always read the full file just to get a single line. The usual approach would be to read the file line by line and then process it accordingly. Or read the file as a whole and process its contents line by line. This would also allow you to get rid of line_count
to keep track of which line you need to look at next. The name of the file to read should probably also be a parameter and not hardcoded. The new version of read_data
might look like:
def read_data(input_file):
"""read from CSV file"""
with open(input_file) as input_data:
csv_reader = csv.reader(input_data, delimiter=',')
return [row for row in csv_reader]
This implementation uses a list comprehension to read all lines of the csv file into memory. With this, count_images()
could simply be replaced by
images_list = read_data(...)
total_images_to_compare = len(images_list)
You could also use a generator expression which is basically a list comprehension, but not all the elements have to be kept in memory at once. Note that with a generator expression the above would not work because len(...)
is not defined for generator expressions.
Either version could be used as follows in the main part of the script:
for image_names in images_list:
# read original image and image to compare into memory
images = read_images(image_names[0], image_names[1])
...
check_identical
Next up on the list is check_identical
. IMHO the implementation here is also a lot more complicated than it has to be. numpy can greatly help to simplifiy the code here:
def check_identical(original_image, compared_image):
"""preliminary check of RGB values are same"""
if original_image.shape == compared_image.shape:
# if images are the same shape and size - could be identical
return np.count_nonzero(original_image - compared_image) == 0
return False
In its core, the function still does what you did, but for the whole image instead of separately for each color channel. The major difference here is that start_time
and image_names
have vanished from the parameter list, since there is reason why check_indentical
should have to care about writing anything to an output file.
tuple unpacking
There are several parts in your code where you do foobar(images[0], images[1])
. If images
has the same number of elements as expected by foobar(...)
, you can simply to foobar(*images)
. This is called tuple unpacking, but also works for other sequences like list
s.
Also, sometimes its more understandable to assign multiple return values to named variables to better see what gets passed around, e.g.
correct_matches, total_matches = filter_results(matches)
score = generate_similarity_score(correct_matches, total_matches)
Sidenote: it would be also possible to do:
score = generate_similarity_score(*filter_results(matches))
Hard-coded values
There are also quite a lot of hard-coded values in your code. Some examples:
'input_test_data.csv'
'result_data.csv'
index_params = dict(algorithm=0, trees=5)
matches = flann.knnMatch(..., k=2)
ratio = 0.6 # approximated through trial and error
IMHO, these all should be function parameters that can be changed without touching the actual code. You can also assign them default values that match their current ones, so nothing in your program has to be changed. At the current state if you ever want to change some of these values, you would have to jump back to where the function is implemented and change it there directly. Though that might sound feasible for a few values, the trouble really starts if you ever get the idea to try several values in a single run (maybe in an effort to optimize a parameter).
The main
function
Although your code boldly states # Main Function
at the end, there is not an actual main
function to be found here. But there definitely should be one! And since we are at it, it's also good practice to make sure the main
function of the script is only called if the file is actually used as a script, and not imported into some other code. Enter the top-level script environment!
# ... all the import and other functions here ...
def main():
"""Read a list of image pairs from disk and check their similarity"""
...
if __name__ == "__main__":
main()
See this SO post for an extended explanation about __name__
.