2
\$\begingroup\$

I've written a python program to rank the names that appear in the file(s) based on their frequency. In other words, there are multiple files and want to rank the frequency of the names that appears the most first, then the least at the end. Each name can appear only once in each file. In other words, the same name cannot be repeated in the same file.

Following is the program, that I've written, and wanted to hear your thoughts, on the style/efficiency and tips to improve the same

from pandas import DataFrame
filenames = [ 'Q1.txt', 'Q2.txt' , 'Q3.txt' , 'Q4.txt']
pathToDataFiles = "C:\\DataFiles\\"
outputHTMLFile =pathToDataFiles + "list.html" 
df = DataFrame( columns = ['Name', 'Q1', 'Q2', 'Q3', 'Q4' ])

currentQtr = 1
for filename in filenames:
    entireFilename = pathToDataFiles  + filename
    # Get all the names from the file
    allNames = [line.rstrip('\n') for line in open(entireFilename)]
    # For each name from the latest .txt file
    for eachname in allNames:
        # If the name is in the dataframe then replace the current quarter with 'Y'
        if ( df['Name'].str.contains(eachname).any() ):
           rowNumber = df.loc[df['Name'] == eachname].index[0]
           df.iloc[[rowNumber],[currentQtr]] = 'Y'
        else:
            # It is a new name, thus, replace the current Qtr with 'Y'. Rest with be Nan
            emptyDataFrame =DataFrame( columns = ['Name', 'Q1', 'Q2', 'Q3', 'Q4' ])
            emptyDataFrame = emptyDataFrame.append([{'Name':eachname}])
            rowNumber = 0
            emptyDataFrame.iloc[[rowNumber], [currentQtr]] = 'Y'
            df = df.append(emptyDataFrame)
            df.index = range(len(df))

    currentQtr = currentQtr + 1
# Create a new column with 'Total' occurence of names 
df['Total'] = currentQtr - df.isnull().sum(axis=1) - 1
# Sort by total
df = df.sort_values(by=['Total'], ascending=False)
df.index = range(len(df))
# Fill all Nan with 'X'
df = df.fillna('X')
df.to_html(outputHTMLFile)
\$\endgroup\$
1
\$\begingroup\$

Here are some suggestions:

Raw Strings

Python has raw strings which make it much easier to use backslashes. The documentation is a bit scattered, but the upshot is that a raw string looks like r'foo' and backslashes don't need to be escaped. This makes raw strings the format of choice for writing Windows paths and regular expressions.

I'd suggest you use something like this:

DATA_DIR = r'C:\DataFiles'

Python Paths

But the fact is that Python file operations will honor windows paths spelled using forward slashes (/) instead of backslashes (\). So you could just as easily define that like this:

DATA_DIR = 'C:/DataFiles'

Use pathlib

Probably the most important thing you could do, however, is to start using pathlib. Don't bother with os.path for this, go directly to the good stuff! The pathlib module is part of the standard library, and does many things that "just work." Including, of course, overloading the operator / to do concatenation:

from pathlib import Path

DATA_DIR = Path('C:/DataFiles')

test = DATA_DIR / 'foo'    # test = Path('C:/DataFiles/foo')

[pro-tip]: Don't hard-code your file names, write code to find them

It seems easy to write something like filenames = [ 'Q1.txt', 'Q2.txt' , 'Q3.txt' , 'Q4.txt'] and it is, until your boss says, "Hey, could you add the last two calendar years to that report?" and you're suddenly stuck trying to move files around and change your code and change the field names in your dataframe and change your HTML and change your spreadsheets and ...

It's better if you have only one source of truth for things like this. In this case, let's use the filenames as the source of truth. Then we can use glob to match them:

from pathlib import Path

DATA_DIR = Path('C:/DataFiles')

def get_data_files(data_path, pattern="*Q[0-9].txt"):
    """ Return a list of data-file Path objects in the given directory
        where the name matches the pattern.

        Note that pattern uses Python glob rules, which allows for
        **/*.txt to recursively traverse subdirectories.
    """
    # NB: Path(string) returns Path, Path(Path) returns Path FTW!
    filepaths = Path(data_dirspec).glob(pattern)
    return filepaths

Writing code like this lets you make one change - renaming Q[1-4].txt to 2019Q[1-4].txt - and you still get the list of paths you need.

Then the rest of the preamble changes to:

quarter_files = get_data_files(DATA_DIR)

for this_q in quarter_files:
    q_name = this_q.stem  # 'Q1', 'Q2', etc.

[pro-tip]: Use with for opening and closing files

There's an example right in the Python documentation for Reading and Writing Files that shows how to use with for file I/O. It's built-in RAII support, if that means anything to you, and it's absolutely the correct way to do file I/O in most cases. Certainly in this one. Plus if you don't use it, every suggestion you get from anyone that (1) reads your code; and (2) knows how to code in Python is going to include "use with for file I/O!". So let's make this quick change:

Change this:

allNames = [line.rstrip('\n') for line in open(entireFilename)]

To this:

with open(this_q) as infile:
    all_names = [line.rstrip('\n') for line in infile]

What's the difference? The difference is that after the with version, the infile is closed. Why does it matter? Two reasons: First, with handles success, failure, exceptions, and always makes sure to close the file. Second, file handles are a surprisingly limited resource, especially if you're debugging code in a REPL. Using with is a good way to never run out.

Let Python do Python

You wrote a lot of code to handle the management of names in your dataframe. But you overlooked the fact that Python has data structures other than dataframes that are easier to work with.

for eachname in allNames:
    # If the name is in the dataframe then replace the current quarter with 'Y'
    if ( df['Name'].str.contains(eachname).any() ):
       rowNumber = df.loc[df['Name'] == eachname].index[0]
       df.iloc[[rowNumber],[currentQtr]] = 'Y'
    else:
        # It is a new name, thus, replace the current Qtr with 'Y'. Rest with be Nan
        emptyDataFrame =DataFrame( columns = ['Name', 'Q1', 'Q2', 'Q3', 'Q4' ])
        emptyDataFrame = emptyDataFrame.append([{'Name':eachname}])
        rowNumber = 0
        emptyDataFrame.iloc[[rowNumber], [currentQtr]] = 'Y'
        df = df.append(emptyDataFrame)
        df.index = range(len(df))

First of all, you claim that names only appear once in any given quarter's data, if they appear at all. So your logic is about checking to see if a name appears in the dataframe because of some previous quarter.

This is a good use for Python's set data type. You can test for containment and compute intersections and differences quite easily.

with open(this_q) as infile:
    names_this_q = {line.rstrip('\n') for line in infile}

That is called a set comprehension, and it works similar to a list comprehension. (And yes, it rocks!) Sets are O(1) for lookup, so you can just use the in operator.

all_names = set()  # Sadly, no literal for empty set

for this_q in quarter_files:
    with open(this_q) as infile:
        names_this_q = {line.rstrip('\n') for line in infile}

    q_name = this_q.stem

    for name in names_this_q:
        if name in all_names:
            # Already in DF
        else:
            # Add it to DF

But you're really either updating an existing record, or accumulating new records. Why not capture all the new records into a python data structure and do one single dataframe update at the end?

An even better question might be to ask if you need to use the dataframe for this at all? You are building what amounts to a boolean table. You could store the boolean values in a data structure built from collections.defaultdict and only convert it into a dataframe for the purpose of generating the HTML at the end.

def quarters_dict():
    """Return a new dict of 'Qx':False for all quarter files Qx."""
    return {qf.stem:False for qf in quarter_files}


all_names = collections.defaultdict(quarters_dict)

for qfile in quarter_files:
    with open(this_q) as infile:
        names_this_q = {line.rstrip('\n') for line in infile}

    q_name = this_q.stem
    for name in names_this_q:
        all_names[name][q_name] = True


And now you have a table of name appearances. You can put `all_names` into a DataFrame and massage it however you like.

Let Pandas do Pandas

Pandas is great at dealing with tables of data of known, basic types. It supports indexing, lookup, boolean, and database type operations. Try to see how you could use that.

First, you are storing Nan and 'Y' values in your columns: floating point and string data? The Nan means false and the 'Y' means true. But there's already a True/False data type, and it is neither floating point nor string. Why not make your table a table of boolean values?

Second, you are treating your names as a column. But they are really the index! So why not make them the index? If you do so, you can use name in df and not have to maintain a separate Python set object.

Third, you are creating one-row dataframes to append values, and then resetting the index of your dataframe. You can just store to a new index! df[new_name] = new_info

Finally, the df.to_html() function takes a large number of parameters. Including formatting parameters that you can use to translate boolean values into 'X' or 'Y'.

Use the features of pandas as they were intended, and your code will get simpler, shorter, and faster.

\$\endgroup\$
  • \$\begingroup\$ Thanks Austin. Appreciate the time and effort for explaining in detail. \$\endgroup\$ – nsivakr Feb 18 at 19:38
1
\$\begingroup\$

I suggest using os.path.join instead of + to handle dangling slashes, when concatenating file names.

This variables:

filenames = [ 'Q1.txt', 'Q2.txt' , 'Q3.txt' , 'Q4.txt']
pathToDataFiles = "C:\\DataFiles\\"

could be better handled by using command line input like in argparse. And for filename you could even use os.listdir to read all files from a directory. Both would give you more flexibility in using this script.

Also, the variable eachname in for eachname in allNames: should be renamed in something like name, to prevent lines like emptyDataFrame = emptyDataFrame.append([{'Name':eachname}]) from looking weird.

There are a few incoherent whitespaces, too, and I think Nan is meant to be NaN (in comments).

Apart from that, it looks like a good start to me. I hope my answer helps and have fun coding!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.