Skip to main content
deleted 1 character in body
Source Link
AlexV
  • 7.3k
  • 2
  • 23
  • 47

IMHO, thesethey 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).

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).

IMHO, they 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).

Source Link
AlexV
  • 7.3k
  • 2
  • 23
  • 47

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 lists.

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__.