2
\$\begingroup\$

I am trying to extract the data from input file and store it for plotting. I have tested this code for a few files of same format. I am not sure if the code works correctly with the little change in input file (like more blank spaces in between). I might have also done some terrible mistakes which I cannot find out while testing. Basically I need this code to be bug free.

Is there any better way of doing this task, improvements and better ways of parsing the file?

Input file:

invoked as: ./kmotif -bin -oracle -seed 12345
random seed = 12345
input: n = 256, m = 399, k = 16 [0.21 ms] {peak: 0.00GiB} {curr: 0.00GiB}
root build ... [zero: 13.81 ms] [pos: 0.02 ms] [adj: 0.02 ms] [adjsort: 0.01 ms] [shade: 0.01 ms] done. [13.87 ms] {peak: 0.00GiB} {curr: 0.00GiB}
command: run oracle
oracle: 0x8E     215.57ms [ 0.250GiB   55.46GiB/s   32.75GHz  144.31GHz] 1 -- true
command done [260.79 ms] 
grand total [274.95 ms] {peak: 0.00GiB}
host: gpu001
build: 32 x GF(2^8) with eight 32-bit words, bit sliced
compiler: gcc 4.4.7

Code:

def parseFile(input, output, mode):
    infile  = open(input)
    outfile = open(output, mode)

    for line in infile:
        if re.match("random seed", line):
            tokens      = re.search(r'random seed = (.*)', line, re.M|re.I)
            seed        = tokens.group(1)
            #print seed

        if re.match("input", line):
            tokens      = re.search(r'(.*)n = (.*), m = (.*), k = (.*) \[(.*) ms\] {peak:(.*)GiB} {curr:(.*)GiB}', line, re.M|re.L)
            n           = tokens.group(2).strip()
            m           = tokens.group(3).strip()
            k           = tokens.group(4).strip()
            t           = tokens.group(5).strip()
            inPeak      = tokens.group(6).strip()
            inCurr      = tokens.group(7).strip()
            #print n, m, k, t, peak, curr

        if re.match("root build", line):
            tokens      = re.search(r'root build (.*)\[zero:(.*)ms\] \[pos:(.*)ms\] \[adj:(.*)ms\] \[adjsort:(.*)ms\] \[shade:(.*)ms\] done. \[(.*)ms\] {peak:(.*)GiB} {curr:(.*)GiB}', line, re.M|re.L)
            zero        = tokens.group(2).strip()
            pos         = tokens.group(3).strip()
            adj         = tokens.group(4).strip()
            adjSort     = tokens.group(5).strip()
            shade       = tokens.group(6).strip()
            rTotalTime  = tokens.group(7).strip()
            rPeak       = tokens.group(8).strip()
            rCurr       = tokens.group(9).strip()
            #print zero, pos, adj, adjSort, shade, rTotalTime, rPeak, rCurr

        if re.match("oracle:", line):
            tokens  = re.search(r'oracle: (.*) (.*)ms \[ (.*)GiB (.*)GiB/s (.*)GHz (.*)GHz\] (.) -- (.*)', line, re.M|re.L);
            sum         = tokens.group(1).strip()
            oracleTime  = tokens.group(2).strip()
            inSize      = tokens.group(3).strip()
            transRate   = tokens.group(4).strip()
            mulRate     = tokens.group(5).strip()
            instrRate   = tokens.group(6).strip()
            yes         = tokens.group(7).strip()
            kpath       = tokens.group(8).strip()
            #print sum, time, inSize, transRate, mulRate, instrRate, yes, kpath

        if re.match("command done", line):
            tokens      = re.search(r'command done \[(.*)ms\]', line, re.M|re.L)
            cmdTime     = tokens.group(1).strip()
            #print cmdTime

        if re.match("grand total", line, re.M|re.L):
            tokens      = re.search(r'grand total \[(.*)ms\] {peak:(.*)GiB}(.*)', line, re.M|re.L)
            totalTime   = tokens.group(1).strip()
            gPeak       = tokens.group(2).strip()
            #print totalTime

        if re.match("host", line):
            tokens      = re.search(r'host:(.*)', line, re.M|re.L)
            host        = tokens.group(1).strip()
            #print host
        if re.match("build:", line):
            tokens      = re.search(r'build:(.*) with(.*)', line, re.M|re.L)
            build       = tokens.group(1).strip()
            #print build

    outfile.write("%s %s %s "% (host, yes, kpath))
    outfile.write(" %6d %8d %2d %10d %8.2lf %8.2lf %8.2lf"%
                    (int(n), int(m), int(k), int(seed), float(t), float(inPeak),
                    float(inCurr)))
    outfile.write(" %8.2lf %8.2lf %8.2lf %8.2lf %8.2lf %8.2lf %8.2lf %8.2lf"%
                    (float(zero), float(pos), float(adj), float(adjSort),
                    float(shade), float(rTotalTime), float(rPeak), float(rCurr)))
    outfile.write(" %s %10.2f %6.3lf %7.2lf %7.2lf %7.2lf"%
                    (sum, float(oracleTime), float(inSize), float(transRate),
                    float(mulRate), float(instrRate)))
    outfile.write(" %8.2lf %8.2lf %8.2lf"%
                    (float(cmdTime), float(totalTime), float(gPeak)))
    outfile.write(" %s\n"% (build))

    #close files
    outfile.close()
    infile.close()
#end of parseFile
\$\endgroup\$
6
  • 1
    \$\begingroup\$ Do you have any known bugs in the code? Do you get the results that you expect from it? \$\endgroup\$ Jul 22, 2015 at 11:48
  • \$\begingroup\$ This doesn't look like Perl. Python maybe? \$\endgroup\$
    – mdfst13
    Jul 22, 2015 at 13:07
  • \$\begingroup\$ sorry, my mistake. It is python... @SimonAndréForsberg few bugs i know is, it wont produce expected results even with minor changes in input file and i get expected results only if the file format exactly matches. \$\endgroup\$
    – 77H3jjuu
    Jul 22, 2015 at 13:12
  • 1
    \$\begingroup\$ I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. Code Review is not the site to ask for help in fixing or changing what your code does. Once the code does what you want, we would love to help you do the same thing in a cleaner way! Please see our help center for more information. \$\endgroup\$ Jul 22, 2015 at 13:16
  • \$\begingroup\$ @SimonAndréForsberg I have updated the code which handles extra blank spaces also. Now there are no known bugs or errors in code. and the program is working as expected. \$\endgroup\$
    – 77H3jjuu
    Jul 22, 2015 at 13:59

1 Answer 1

4
\$\begingroup\$

First things first - I really don't enjoy the style

tokens      = re.search(r'(.*)n = (.*), m = (.*), k = (.*) \[(.*) ms\] {peak:(.*)GiB} {curr:(.*)GiB}', line, re.M|re.L)
n           = tokens.group(2).strip()
m           = tokens.group(3).strip()
k           = tokens.group(4).strip()
t           = tokens.group(5).strip()
inPeak      = tokens.group(6).strip()
inCurr      = tokens.group(7).strip()
#print n, m, k, t, peak, curr

I find it hard to read, and most/many people I work with would also flag this in a code review.

Moving on to more substantive things.

Use a context manager

Instead of using

infile  = open(input)
outfile = open(output, mode)

use

with open(input, 'r') as infile, open(output, mode) as outfile:
    ...

This allows you to safely do anything you want and guarantee that even if an exception is raised your files will still be closed.

I also tend to prefer being explicit about the mode in which my file is opened, so I added the 'r'.

Don't mask Python builtins

You use input and sum - both are builtin functions that are masked locally by your variables - if you must use an existing name, append a _ to the variable name (i.e. input_ and sum_)

Compile your regexes

Compiling your regular expressions, contrary to popular belief, generally won't actually speed up your code (at least in 2.5 all regexes are compiled and cached internally, and I assume this continues to later versions). Instead it provides you with error checking, and a name to assign to your regex

Use re.VERBOSE

You have a lot of stuff happening in these regexes, and when you look back (or someone else looks at this) it can take a while to read through what each regex does, and why.

Avoid regular expressions where possible

Almost all of your conditions looks like line.startswith("some string") would be more appropriate in terms of readability and speed. The one care where you include some flags (grand total) should have a comment explaining why.

Use functions

You basically have 8 distinct operations that occur in your for loop. Separating them out makes it easier to test the one you want, and makes parseFile much cleaner.

Be careful

You assume that a lot of variables have been declared at the end of your file - if you are very confident that your file will always be valid then that's okay, but be warned that this makes for a rather fragile parse function. An alternative would be to do something like locals().get(var_name, default) instead to avoid any unexpected exceptions.

Documentation

There is no documentation here, which makes it hard for you to maintain this, and even harder for someone else to join the project. A file-level docstring would be appropriate (documenting the purpose of the function, expected input file format and output file format, etc). Then specific function docstrings to explain what each one does. Lastly in-line comment sto explain what particular bits do (i.e. the regular expressions, although as I mentioned re.VERBOSE is probably a better way to explain those

Use elif

Instead of checking each if block, use elif so it'll stop once it reaches the expected block.

Write tests

You said you needed your code to be bug free, and while proving code is correct is difficult (how good is your Hoare logic?), you can generally demonstrate that a program is mostly not incorrect by writing test cases. There are a lot of ways to do this in Python - either the builtin unittest module, or the 3rd-party libraries nose and pytest (there are more, but I've never used them). Breaking each regex out into a separate function makes it easier to test those individual regexes for acceptance, and then you can also test the program as a whole with a ton of valid and invalid cases.

Altogether

import re

def _random_seed(line):
    regex = re.compile(r'random seed = (.*)', re.M | re.I | re.X)
    return regex.search(line).group(1).strip()

def _input(line):
    regex = re.compile(r'(.*)n = (.*), m = (.*), k = (.*) \[(.*) ms\] {peak:(.*)GiB} {curr:(.*)GiB}',  re.M | re.I) | re.X)
    tokens = regex.search(line)
    return map(lambda string: string.strip(), map(tokens.group, xrange(2, 8)))

def _root_build(line):
    regex = re.compile(r'root build (.*)\[zero:(.*)ms\] \[pos:(.*)ms\] \[adj:(.*)ms\] \[adjsort:(.*)ms\] \[shade:(.*)ms\] done. \[(.*)ms\] {peak:(.*)GiB} {curr:(.*)GiB}', re.M | re.L | re.X)
    tokens = regex.search(line)
    return map(lambda string: string.strip(), map(tokens.group, xrange(2, 9)))

def _oracle(line):
    regex = re.compile(r'oracle: (.*) (.*)ms \[ (.*)GiB (.*)GiB/s (.*)GHz (.*)GHz\] (.) -- (.*)', re.M | re.L | re.X)
    tokens = regex.search(line)
    return map(lambda string: string.strip(), map(tokens.group, xrange(1, 9)))

def _command_done(line):
    regex = re.compile(r'command done \[(.*)ms\]', re.M | re.L | re.X)
    return regex.search(line).group(1).strip()

def _grand_total(line):
    regex = re.compile(r'grand total \[(.*)ms\] {peak:(.*)GiB}(.*)', re.M | re.L | re.X)
    tokens = regex.search(line)
    return map(lambda string: string.strip(), map(tokens.group, xrange(1, 3)))

def _host(line):
    regex = re.compile(r'host:(.*)', re.M | re.L | re.X)
    return regex.search(line).group(1).strip()

def _build(line):
    regex = re.compile(r'build:(.*) with(.*)', re.M | re.L | re.X)
    return regex.search(line).group(1).strip()

def parseFile(input_, output, mode):
    with open(input_, 'r') as infile, open(output, mode) as outfile:
        for line in infile:
            if line.startswith("random seed"):
                tokens = _random_seed(line)
            elif line.startswith("input"):
                n, m, k, t, inPeak, inCurr = _input(line)
            elif line.startswith("root build"):
                zero, pos, adj, adjSort, shade, rTotalTime, rPeak, rCurr = _root_build(line)
            elif line.startswith("oracle:"):
                sum_, oracleTime, inSize, transRate, mulRate, instrRate, yes, kpath = _oracle(line)
            elif line.startswith("command done"):
                cmdTime = _command_done(line)
            elif re.match("grand total", line, re.M|re.L):
                totalTime, gPeak = _grand_total(line)
            elif line.startswith("host"):
                host = _host(line)
            elif line.startswith("build"):
                build = _build(line)

        outfile.write("%s %s %s "% (host, yes, kpath))
        outfile.write(" %6d %8d %2d %10d %8.2lf %8.2lf %8.2lf"%
                        (int(n), int(m), int(k), int(seed), float(t), float(inPeak),
                        float(inCurr)))
        outfile.write(" %8.2lf %8.2lf %8.2lf %8.2lf %8.2lf %8.2lf %8.2lf %8.2lf"%
                        (float(zero), float(pos), float(adj), float(adjSort),
                        float(shade), float(rTotalTime), float(rPeak), float(rCurr)))
        outfile.write(" %s %10.2f %6.3lf %7.2lf %7.2lf %7.2lf"%
                        (sum, float(oracleTime), float(inSize), float(transRate),
                        float(mulRate), float(instrRate)))
        outfile.write(" %8.2lf %8.2lf %8.2lf"%
                        (float(cmdTime), float(totalTime), float(gPeak)))
        outfile.write(" %s\n"% (build))

You'll notice a few changes here I mentioned but didn't make, and a few I made but didn't mention. Firstly, although I added re.X (equivalent to re.VERBOSE) I didn't actually comment any of them (in fact I didn't comment or document anything - that is something you can presumably do much better than I). I also didn't do the locals().get(var_name, default) thing - the usefulness of that is debatable, and depends on how confident you are in your files being correct. I also didn't write tests for you - writing comprehensive and good tests takes time and skill, and are easier for you (as you'll know the actual requirements of your project).

I also used functional programming in the helper functions - using map, lambda functions and xrange I was able to reduce code repetition. You could make an even more generic function like

def get_values(pattern, flags, line, range_start, range_end):
    tokens = re.search(pattern, line, flags)
    return map(lambda string: string.strip(), map(tokens.group, xrange(range_start, range_end)))

and then just call that for each iteration. You could probably keep extending on that idea and make a more and more generic solution that would reduce how much code is required. For example, creating a tuple like this

conditions = (
    (some_validation_function, regex, flags, range_start, range_end),
    ...
)

could turn your for loop into

for line in infile:
    for condition in conditions:
        if condition[0](line):
            return _get_token(line, *condition[1:])

although this would require some reorganization of how you handle the code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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