Adjusting all depth data in a file by some elevation

Question

The code adds up numbers from two different text files. It opens an input file 'DEPTH.dat' and reads the numbers after some headers as follows:

SCALAR
ND    3
ST    0
TS    1000
1.0
1.0
1.0
SCALAR
ND    3
ST    0
TS    2000
3.3
3.4
3.5
SCALAR
ND    3
ST    0
TS    3000
1.7
1.8
1.9

It then adds to them numbers which have been read from a smaller text file 'ELEVATION.dat' which looks as follows:

SCALAR
ND    3
ST    0
TS    0
10.0
10.0
10.0

The result is written in a text file 'output.txt' like this:

SCALAR
ND    3
ST    0
TS    1000
11.0
11.0
11.0
SCALAR
ND    3
ST    0
TS    2000
13.3
13.4
13.5
SCALAR
ND    3
ST    0
TS    3000
11.7
11.8
11.9

---

Here's the code in question. As I have learned from 200_success in my last code review that I should write the code using functions, I tried to write this new code using some functions.

For a small example, the code works properly but for bigger input data, it does not work. (For a 350 MB file with 30 million lines, there is no response after two hours.) How can the code be improved?

from itertools import zip_longest


def grouper(iterable, n, padvalue=None):
    return zip_longest(*[iter(iterable)]*n, fillvalue=padvalue)


def writing_ND(f1):
    for line1 in f1:
        if line1.startswith('ND'):
            ND = float(line1.split()[-1])
            return ND


def writing_TS(f):
    with open(f, 'r') as f:
        for line1 in f:
            if line1.startswith('TS'):
                x = float(line1.split()[-1])
                TS.append(x)
        return TS
TS = []
ND = []
n = 0
add_numbers = []
TS = writing_TS('DEPTH.dat')
with open("ELEVATION_LHP.dat") as f, open("DEPTH.dat") as f1,\
     open('output.txt', 'w') as out:
    ND = writing_ND(f)
    n = int(ND)+4
    f.seek(0)
    for lines in grouper(f, int(n)):
        for item in lines[4:]:
            add_numbers.append(float(item))
    i = 0
    for l in grouper(f1, n):
        data_numbers = []
        for line in l[4:]:
            data_numbers.append(float(line.split()[-1].strip()))
            result_numbers = [x + y for x, y in zip(data_numbers, add_numbers)]
        del data_numbers
        out.write('SCALAR\nND    %d\nST  0\nTS      %0.2f\n' % (ND, TS[i]))
        i += 1
        for item in result_numbers:
            out.write('%s\n' % item)

Answer 1

Minor issues

You have some minor funny business going on:

• Repurposing f in writing_TS() makes the code harder to follow:

  with open(f, 'r') as f:
  

• In writing_TS(), you do

  TS.append(x)
  

  … where you are modifying TS, which is a global variable. But then you return TS as well, which makes it pointless to communicate via a global variable.

• On the other hand, you also have a global variable ND, which is initially an empty list, but becomes a float, but you actually want it to be an int. But why bother initializing n = 0 at all?

Performance

Your ELEVATION.dat is a small file, and your DEPTH.dat is a huge file. So don't process DEPTH.dat twice! You want to run through DEPTH.dat linearly, in a single pass.

When performance is a problem, just keep it as simple as possible. The core of the solution should "ideally" resemble this:

with open('DEPTH.dat') as depth, open('OUTPUT.dat', 'w') as out:
    for line in depth:
        out.write(line)

Of course, that doesn't do anything useful — it just copies the file line by line. But it gives you the outline of the code to strive for, and it also establishes a lower bound on the running time. (Try it. If you aren't satisfied with the performance, then you know that your task cannot be accomplished satisfactorily using Python.)

With that in mind, how can we modify the skeleton to actually do the work? I would make two assumptions:

• DEPTH.dat consists of repeated blocks resembling the structure of ELEVATION.dat.
• The lines of DEPTH.dat should be copied verbatim, except the numeric lines, which should be summed with their counterpart in ELEVATION.dat.

Here is how I would express those two ideas in code:

import itertools

def read_elevation(filename):
    """
    Read elevation data.  Lines in the file that consist only of a float are
    converted to a float; all other lines are preserved.
    """
    def try_parse_float(line):
        try:
            return float(line)
        except ValueError:
            return line

    with open(filename) as f:
        return [try_parse_float(line) for line in f]


elevation_cycle = itertools.cycle(read_elevation('ELEVATION_LHP.dat'))
with open('DEPTH.dat') as depth, \
     open('OUTPUT.dat', 'w') as out:
    for depth_line, elev in zip(depth, elevation_cycle):
        if isinstance(elev, float):
            print(elev + float(depth_line), file=out)
        else:
            out.write(depth_line)

This is simpler than your original code, in that I'm not trying as hard to "make sense" of the data by parsing ND. It's not assuming that there are four header lines per group, namely SCALAR, ND, ST, and TS. There isn't even any grouping involved: it just blindly assumes that DEPTH.dat contains many repetitions of blocks resembling ELEVATION.dat.

Answer 2

Well that's a lot of input data, but also the processing isn't super optimised, so I'm not that surprised it takes a bit long.

For the start I'd like to clean up a bit, writing_ND and writing_TS should be similar and probably do the seek call in the functions, so I'd suggest something like the following:

def writing_ND(f):
    try:
        for line in f:
            if line.startswith('ND'):
                return float(line.split()[-1])
    finally:
        f.seek(0)


def writing_TS(f):
    result = []
    try:
        return [float(line.split()[-1])
                for line in f if line.startswith('TS')]
    finally:
        f.seek(0)

Note the addition of try/finally to always reset the file back to the start and the use of a list comprehension to make the loop more compact.

For the main function (well, "function", there's no main right now) I'd suggest moving variables close to their usage and to not "declare" variables like ND, TS and n like that, just assign them on first use with the correct values:

with open('ELEVATION_LHP.dat') as f, open('DEPTH.dat') as f1, open('output.txt', 'w') as out:
    ND = writing_ND(f)
    TS = writing_TS(f1)
    n = int(ND) + 4

Of course f and f1 aren't so great names. The next loop looks okay:

add_numbers = []
for lines in grouper(f, n):
    for item in lines[4:]:
        add_numbers.append(float(item))

i shouldn't be necessary, using enumerate is clearer. The indentation for result_numbers is probably wrong, otherwise the repeated calls are unnecessary. I'd probably also try not to construct the intermediate lists for data_numbers or result_numbers - while that's nice for readability it'll produce some garbage and therefore will take more time. Maybe start with this:

for i, l in enumerate(grouper(f1, n)):
    out.write('SCALAR\nND    %d\nST  0\nTS      %0.2f\n' % (ND, TS[i]))
    for line, number in zip(l[4:], add_numbers):
        out.write('%s\n' % (float(line.split()[-1].strip()) + number))

Lastly, for grouper I'd remove the padvalue parameter as it's unused (shouldn't this be in a library somewhere anyway?)

---

At this point I'd suggest to take a profiler and measure where exactly most of the time is taken. As a really simple thing while developing I'd even just add a progress output say every thousand input records or so, simply to have some idea about whether the program is actually progressing or not.

I imagine the line splitting, float conversion, or output formatting might be part of the problem. There are a couple of routes to take afterwards, mostly by avoiding work, or replacing convenient functions (split) with manual work.

---

Lastly, this approach seems, while working, a bit complicated. Is there no way that you can use a more compact format, or keep the values in-memory for later processing? Especially having a single pandas data frame (or NumPy even) makes life much easier if you can stay in Python.