Average spam confidence

Question

Exercise 7.2 from Python for Informatics:

Write a program to prompt for a file name, and then read through the file and look for lines of the form:

X-DSPAM-Confidence: 0.8475

When you encounter a line that starts with “X-DSPAM-Confidence:” pull apart the line to extract the floating-point number on the line. Count these lines and then compute the total of the spam confidence values from these lines. When you reach the end of the file, print out the average spam confidence.

Enter the file name: mbox.txt
Average spam confidence: 0.894128046745

Enter the file name: mbox-short.txt
Average spam confidence: 0.750718518519

Test your file on the mbox.txt and mbox-short.txt files.

My code works. It's a bit more forgiving with the amount of decimals on both input and output, but that's as intended.

What I'm not happy about are the ask_file_name and retrieve_values functions. The first uses a hacky method of input validation (the Path module seems appropriate here, but that would be overkill), the second uses iteration while it shouldn't. Perhaps a reduce would be more appropriate, or something different altogether.

This is written in modern Python. Which means it's written in Python 3 (3.5.2 to be exact, Python 3.7.x is incoming) and has plenty of docstrings. It should be up to standards, but I'd like to have that verified.

AverageSpamConfidence.py

#! /usr/bin/env python3
# coding: utf-8

# Sample data from http://www.py4inf.com/code/

import re


def ask_file_name():
    """
    Ask user for file name.

    Check for FileNotFound & IsADirectory errors.
    Keyword arguments:
    -
    """
    while True:
        try:
            user_input = input("Enter the file name: ")
            # Is the file there and can we open it?
            with open(user_input, "r") as test_input:
                pass
        except FileNotFoundError:
            print("Input empty or file does not exist.")
            continue
        except IsADirectoryError:
            print("That's not a directory, not a file.")
            continue
        else:
            return user_input


def find_occurences_in_file(file_name):
    """
    Find all occurences in target file.

    Keyword arguments:
    file_name -- name of (and path to) target file (example: mbox.txt)
    """
    with open(file_name, "r") as input_file:
        return re.findall(
            'X-DSPAM-Confidence: 0.[0-9]+', str(input_file.readlines())
        )


def retrieve_values(input_list):
    """
    Return relevant values from list.

    Keyword arguments:
    input_list -- list to results to retrieve values from
    """
    return_list = []
    for line in input_list:
        return_list.append(float(line.split()[-1]))
    return return_list


def average(input_list):
    """
    Calculate average of list provided.

    Keyword arguments:
    input_list -- list of numerical values
    """
    return sum(input_list) / len(input_list)


def main():
    """
    Exercise 7.2 (Python for Informatics, Charles Severance)
    Write a program to prompt for a file name,
    and then read through the file and look for lines of the form:

    X-DSPAM-Confidence: 0.8475

    When you encounter a line that starts with “X-DSPAM-Confidence:” pull apart
    the line to extract the floating-point number on the line.
    Count these lines and then compute the total of the spam confidence values
    from these lines. When you reach the end of the file, print out
    the average spam confidence.
    """
    file_name = ask_file_name()
    occurences = find_occurences_in_file(file_name)
    values = retrieve_values(occurences)
    print(average(values))

if __name__ == '__main__':
    main()

Answer 1

Not duplicating any of @Peilonrayz's code review points ...

---

Stop reading entire files into memory when you can process the file line by line in one pass, and stop creating huge lists in memory which are then iterated over exactly once. Both of these things creates a huge unnecessary memory pressure which can be avoided by looping and/or using generator expressions.

Using a simple loop over all lines in the file:

def average_spam_confidence(filename):
    with open(filename) as file:
        count = 0
        total = 0
        for line in file:
            if line.startswith("X-DSPAM-Confidence: "):
                try:
                    total += float(line[20:])
                    count += 1
                except:
                    pass   # Line ended with garbage
        return total / count

No regex. No reading entire file in memory. No creating a list of individual confidence values to sum up afterwards.

Plus, we've fixed a bug! If "X-DSPAM-Confidence:" appears in the middle of a line, instead of at the start as required by the problem text, we don't try to process it.

But perhaps you wanted a more functional way of programming, where you:

1. find all the confidence lines,
2. extract the confidence values, and
3. compute the average

... all as separate steps which you can compose together, and reuse to solve other problems. Fear not! We can do that too! Enter generator expressions:

First, let's open the file, and read all the matching lines into a list, using list comprehension:

with open(filename) as file:
    lines = [line for line in file if line.startswith("X-DSPAM-Confidence: ")]

That second statement loops through each line in the file, checks if the line begins with the desired text, and if so, includes it in the list that is being constructed. We can later iterator over lines to further process each line individually.

That is almost what we want to do. Well, that is exactly what we want to do, but we don't want to do it all at once. If we change the [...] to (...), we move from list comprehension to a generator expression.

with open(filename) as file:
    lines = (line for line in file if line.startswith("X-DSPAM-Confidence: "))

Now, the second statement has done ... nothing. We haven't read the first character of the file yet. What we have returned is a generator expression, which when we ask for the first value will start reading lines until it finds one that matches, and then it will pause its execution and return that value.

Ok. Let's extract just the confidence values:

    values = [ line[20:] for line in lines ]

Whoops! That's list comprehension. It will loop over all the lines that the lines generator can produce, skip over the prefix and return the rest. Again, change those [...] to (...):

    values = ( line[20:] for line in lines )

Better! Now those are still strings, so we'll need to convert them into float point values. Too easy. Just map them:

    confidences = map(float, values)

confidences is a generator. If you said list(confidences), you'd create that in-memory list of float values for all the "X-DSPAM-Confidence:" values in the file. And you could then sum(...) and len(...) the list of values to compute the average. But we don't want to realize the list in memory, so ...

def average(data):
    total = 0
    count = 0
    for value in data:
        total += value
        count += 1
    return total / count

average_confidence = average(confidences)

... we ask for values from the confidences generator, add them up one at a time, counting as we go. When the generator is exhausted, the for loop ends, and we return the average.

Putting it all together:

def average(data):
    total = 0
    count = 0
    for value in data:
        total += value
        count += 1
    return total / count

def average_spam_confidence(filename):
    with open(filename) as file:
        lines = (line for line in file if line.startswith("X-DSPAM-Confidence: "))
        values = ( line[20:] for line in lines )
        confidences = map(float, values)
        return average(confidences)

Or more simply:

import statistics

def average_spam_confidence(filename):
    with open(filename) as file:
        values = (line[20:] for line in file
                            if line.startswith("X-DSPAM-Confidence: "))
        return statistics.mean(map(float, values))

Note: the non-generator solution was more robust converting the confidence value strings into floats, via a try...except block. The generator expression solution shown above omits that. The robustness may be improved by using a more precise matching when searching for the "X-DSPAM" lines (regex). Alternately, a generator function could be used, which discards the non-float values.

def map_to_float(data):
    for value in data:
        try:
            yield float(value)
        except:
            pass

confidences = map_to_float(values)

---

Note (from @Roland Illig's comment): Because generator expressions delay the execution of their operation, any resources they use must remain available until their processing has been finished. They cannot be used to compute the average spam confidence if the file they are reading from is closed before the average has been computed. In the above examples, the generator expressions are fully consumed within the body of the with open(...) as file: block, so the file was kept open.

This does not mean the generator expressions must all occur within the with statement. They can be spread out across many functions, but their execution must be constrained to the interval when the file is open:

def find_occurrences_in_file(input_file):
    """Return a generator for occurrences of X-DSPAM lines"""
    return (line for line in input_file if line.startswith("X-DSPAM-Confidence: "))

def retrieve_values(input_list):
    """Return a generator for float values from a list"""
    return map(float, (line.split()[-1] for line in input_list))

def average(input_list):
    """Compute average of a list/sequence/iterable/generator of values..."""
    return statistics.mean(input_list)

def average_spam_confidence(file_name):
    with open(file_name) as file:
        # File is open for this entire with statement.
        occurrences = find_occurrences_in_file(file)
        values = retrieve_values(occurrences)
        print(average(values))
        # File is closed here - but "average(...)" has exhaustively read
        # from all of the generators already.

Answer 2

• I think ask_file_name looks fine without using pathlib. The difference between the two comes down to LBYL vs EAFP.

  For the most part the difference between the two is style. Do you prefer using except FileNotFoundError or an if path.exists().

• What I do find strange is ask_file_name follows a LBYL approach, but the code inside it follows an EAFP approach.

  To make it fully EAFP return the file object you create in the try.

• I'd move the input call outside the try. Whilst it's unlikely to raise either error, it's a good habit to get into.

• There's no need to continue in the except, this is as there are no statements after the try.

• I don't understand why you've used str(input_file.readlines()).

  This converts from a list to a string adding additional noise. You can also read the entire file with input_file.read().

• retrieve_values can be changed to a list comprehension.

• Don't mix string delimiters, pick either " or '.
• I'm not a fan of your names. I have changed them to what I would use, but you may dislike my naming style.

_{Docstrings removed for brevity}

#! /usr/bin/env python3
# coding: utf-8

# Sample data from http://www.py4inf.com/code/

import re


def get_file():
    while True:
        path = input('Enter the file name: ')
        try:
            return open(path, 'r')
        except FileNotFoundError:
            print('Input empty or file does not exist.')
        except IsADirectoryError:
            print("That's not a directory, not a file.")


def find_confidences(file):
    return re.findall(
        'X-DSPAM-Confidence: 0.[0-9]+',
        file.read()
    )


def retrieve_confidences(confidences):
    return [
        float(confidence.split()[-1])
        for confidence in confidences
    ]


def average(values):
    return sum(values) / len(values)


def main():
    with get_file() as file:
        occurences = find_confidences(file)
    confidences = retrieve_confidences(occurences)
    print(average(confidences))


if __name__ == '__main__':
    main()