4
\$\begingroup\$

I have a text file composed of several subsequent tables. I need to get certain values from certain tables and save them in an output file. Every table has a header which contains a string that can be used to find specific tables. The size of these text files can vary from tenths of MB to some GB. I have written the following script to do the job:

string = 'str'
index = 20
n = 2

in_file = open('file.txt')
out_file = open("out.txt", 'w')
current_line = 0

for i in range(-index,index+1):
    for j in range(-index,index+1):
        for line in in_file:
            if string in line:
                En = line.split().pop(4)
                for line in in_file:
                    current_line += 1
                    if current_line == 2*(n+1)+2:
                        x = line.split().pop(10)
                    elif current_line == 3*(n+1)+2:
                        y = line.split().pop(10)
                    elif current_line == 4*(n+1)+2:
                        z = line.split().pop(10)
                        current_line = 0
                        break
                print i, j, En, x, y, z
                data = "%d %d %s %s %s %s\n" % (i,j,En,x,y,z)
                out_file.write(data)
                break
in_file.close()
out_file.close()

The script reads the file line by line searching for the specified string ('str' in this example). When found, it then extracts a value from the line containing the string and continue reading the lines that form the data table itself. Since all the tables in the file have the same number of lines and columns, I've used the variable current_line to keep track of which line is read and to specify which line contains the data I need. The first two for-loops are just there to generate a pair of indexes that I need to be printed in the output file (in this case they are between -20 and 20).

The script works fine. But since I've been learning python by myself for about one month, and the files I have to handle can be very big, I'm asking for advices on how to make the script more efficient, and overall, better.

Also, since the tables are regular, I can know beforehand which are the lines that contain the values I need. So I was wondering, instead of reading all the lines in the file, is it possible to specify which lines have to be read and then jump directly between them?

Sample input file

Here's a sample input file. I've included just some tables so you can have an idea how it's organized. This file is composed by two blocks with three tables each. In this sample file, the string "table #" is what is used to find the data to be extracted.

Sample output file

And here's a sample output file. Keep in mind that these two files are not equivalent! This output was created by my script using an input file containing 1681 blocks of 16 tables. Each table had 13 lines just as in the sample input file.

\$\endgroup\$
6
\$\begingroup\$

All this nesting obfuscates your code pretty badly, so let's start by reducing it so it's easier to read. If you need to test something in your for loop, you should flip the test and use continue, like this:

    for line in in_file:
        if not string in line:
            continue
        En = line.split().pop(4)

This saves you one block of nesting at least. You could also combine your two range calls with itertools.product. product performs a nested loop based on the two iterables it's passed. It essentially creates the exact nested loop you have with just one line:

for i, j in itertools.product(range(-index,index+1), range(-index,index+1)):
    for line in in_file:
        if not string in line:
            continue
        En = line.split().pop(4)
        for line in in_file:
            current_line += 1
            if current_line == 2*(n+1)+2:
                x = line.split().pop(10)
            elif current_line == 3*(n+1)+2:
                y = line.split().pop(10)
            elif current_line == 4*(n+1)+2:
                z = line.split().pop(10)
                current_line = 0
                break
        print i, j, En, x, y, z
        data = "%d %d %s %s %s %s\n" % (i,j,En,x,y,z)
        out_file.write(data)
        break

It's certainly more readable now. Though still confusing, why are you looping over in_file within the in_file nested group? It seems redundant but also I think you have misunderstood how file reading works. When you have read the first ten lines of a file object and then start a new loop over that same object you wont re-read the first ten lines. This means there's no reason to nest your inner for loop like this. This will iterate over the same values:

for i, j in itertools.product(range(-index,index+1), range(-index,index+1)):
    for line in in_file:
        if not string in line:
            continue
        En = line.split().pop(4)
    for line in in_file:
        current_line += 1

Now about that current_line, you actually don't need to use that, you could use enumerate instead to automatically count the iteration you're on. enumerate takes an iterable and returns both the value, ie. line but also a number which you can use to replace current_line. enumerate usually starts counting from 0, but you can pass an optional starting parameter:

    for current_line, line in enumerate(in_file, 1):

I think current_line is a confusing name especially now that it's next to line. index or i are what I'd use, personally.

Also you're creating the same string twice, once to print and once to write. Just create data upfront so you can print the same value, this will avoid confusion with typo discrepancies between the two. You can manually add the newline in the write call instead. You should also use the new way of formatting, str.format, as it's type agnostic:

        data = "{} {} {} {} {}".format(i, j, En, x, y, z)
        print (data)
        out_file.write(data + '\n')

Now let's see how this looks with these changes:

for i, j in itertools.product(range(-index,index+1), range(-index,index+1)):
    for line in in_file:
        if not string in line:
            continue
        En = line.split().pop(4)
        break
    for idx, line in enumerate(in_file, 1):
        if idx == 2*(n+1)+2:
            x = line.split().pop(10)
        elif idx == 3*(n+1)+2:
            y = line.split().pop(10)
        elif idx == 4*(n+1)+2:
            z = line.split().pop(10)
            break

    data = "{} {} {} {} {}".format(i, j, En, x, y, z)
    print (data)
    out_file.write(data + '\n')

Now, a note about using open for your files. It's actually better to use with because it will always ensure your files are closed no matter what, even if error's occur during your script. It will unfortunately reintroduce nesting, but it it is worth it.

with open("file.txt") as in_file, open("out.txt", "w") as out_file:
    for i, j in itertools.product(range(-index,index+1), range(-index,index+1)):

Your names could definitely use work too. x, y and z don't mean anything to me. What is n? Is it a constant? Does it change? string is likewise vague, something like key would make more sense. index is confusing too, as it's only for specificying the range you loop over. Would boundary work perhaps? The problem is, your names are so vague I can't even suggest better ones because I don't know what these things do.

Also you should match Python naming conventions. You use snake_case for current_line which is good, but En looks like a class due to the capital. It should just be en, or better yet a descriptive name. Even i and j would benefit from descriptive names if it was possible.

\$\endgroup\$
  • \$\begingroup\$ An efficiency question for you, @SuperBiasedMan, using line.split().pop(10) vs line.split()[10]. Doesn't the former modify the temporary list made by split(), whilst the latter just fetches the 10th index? And so is a tiny fraction faster, and to me a little easier to read? \$\endgroup\$ – holroy Oct 14 '15 at 11:53
  • \$\begingroup\$ I tested quickly with timeit and you're right that it's faster to just get the index. I personally find it more readable too, so you could write an answer about it. And thanks! \$\endgroup\$ – SuperBiasedMan Oct 14 '15 at 12:03
  • \$\begingroup\$ Thank you very much for your answer. I actually didn't know any of the things you suggested. I will take some time to understand what the commands do and test them. About the names in the code, I tried to make then as generic as possible when posting the code because some of them (the real 'str', for example) are very specific and wouldn't mean anything out of context. In the specific case of 'n', it's a constant within a file but it can change between files. If you take a look at the sample input file I provided, 'n' corresponds to the # of elements. So each table will have 4(n+1)+1 lines. \$\endgroup\$ – Yeshua Oct 14 '15 at 13:05
  • \$\begingroup\$ @Yeshua Glad to help! If you do feel like making names more general in future it's helpful to note that in your question so that reviewers will know they don't need to be commented on. \$\endgroup\$ – SuperBiasedMan Oct 14 '15 at 13:12
  • \$\begingroup\$ @SuperBiasedMan has offered excellent suggestions. I suggest adding a lint or code checking method to your IDE (if you use VIM you can use PEP8 or pyflakes or pymode addons to VIM). This will offer you suggestions on readability and dead code. It can also help you learn conventions such as Class names, CONSTANT names, and methodNames. \$\endgroup\$ – cgseller Oct 15 '15 at 0:09
3
\$\begingroup\$

In addition to the excellent answer by SuperBiasedMan, I would like to make a small point regarding the use of line.split().pop(10) vs another possibility line.split()[10], and precalculation of line numbers.

pop(n) vs [n]

Both cases creates a temporary list of the words in the line, due to line.split(), but there is a slight difference after that. Using .pop(10) will actually remove the 10th element of the list, change the index of all preceding list elements, and return the popped element. Using [10] will simply give you the 10th element.

It maybe a marginal performance issue, but additionally the index variation is somewhat easier to read and understand. While pop() has it advantages, in this context it just seemed a little misplaced.

Precalculation of if condition parts

In your code you are calculating the line numbers each and every time you hit the if statements. However the calculation is a constant throughout your script. You could do something like the following at start of the script:

x_line_number = 2*(n+1) + 2
y_line_number = 3*(n+1) + 2
z_line_number = 4*(n+1) + 2

# Replace for loop with this    
for idx, line in enumerate(in_file, 1):
    if idx == x_line_number:
        x = line.split()[10]
    elif idx == y_line_number:
        y = line.split()[10]
    elif idx == z_line_number:
        z = line.split()[10]
        break

Might yet again not be a big issue, but in the general case when doing multiple loops it can often be an advantage to precalculate the constant part of if conditions. The heavier the calculation is, the more important it is to do it ahead of time.

\$\endgroup\$
  • \$\begingroup\$ I second the precalculation because regardless of performance, it actually reads clearer. Calculating on each iteration implies that the value changes contextually, I didn't even realise they'd be the same every time. \$\endgroup\$ – SuperBiasedMan Oct 14 '15 at 13:10
  • \$\begingroup\$ Thank you for your advices and explanations. They will help me to write better codes from now on. \$\endgroup\$ – Yeshua Oct 14 '15 at 13:16

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.