First version of gpuveto testing script

tools/test_gpuveto.py

+#!/usr/bin/env python
+
+# Copyright 2014 Google Inc.
+#
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""

[rmistry, 2014/04/28 16:58:44]

"""Script to test out suitableForGpuRasterization (via gpuveto)."""

[robertphillips, 2014/04/28 17:45:58]

Done.

+Script to test out suitableForGpuRasterization (via gpuveto)
+"""
+
+import glob
+import os
+import re
+import subprocess
+import sys
+
+# Set the PYTHONPATH to include the tools directory.
+sys.path.append(
+    os.path.join(os.path.dirname(os.path.realpath(__file__)), os.pardir))
+import find_run_binary
+
+USAGE = """
+Usage:
+    {command} SKP_FILE
+    {command} SKP_DIR\n
+"""
+
+def find_path_to_program(program):
+    """
+    Returns path to an existing program binary.

[rmistry, 2014/04/28 16:58:44]

Keep this line above, eg:
"""Returns path to an existing program binary.

...
"""

The first line is a one-line description of the function similarly everywhere
below as well.
From python's style guide:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments

[robertphillips, 2014/04/28 17:45:58]

Done.

+
+    @param program: Basename of the program to find (e.g., 'render_pictures').
+
+    @returns absolute path to the program binary, as a string.
+
+    Raises:
+        Exception: unable to find the program binary.
+    """
+    return find_run_binary.find_path_to_program(program)
+

[rmistry, 2014/04/28 16:58:44]

Nit: Need 2 newlines between top level definitions (here and everywhere below).

[robertphillips, 2014/04/28 17:45:58]

Done.

+def list_files(dir_or_file):
+    """
+    Accepts a directory or filename. Returns a list of all the files in the
+    directory or just the file.
+    """

[rmistry, 2014/04/28 16:58:44]

Change above to:

"""Returns list of all files from the provided argument.

@param dir_or_file: ....

@returns ...
"""

[robertphillips, 2014/04/28 17:45:58]

Done.

+    files = []
+    for globbedpath in glob.iglob(dir_or_file): # useful on win32
+        if os.path.isdir(globbedpath):
+            for filename in os.listdir(globbedpath):
+                newpath = os.path.join(globbedpath, filename)
+                if os.path.isfile(newpath):
+                    files.append(newpath)
+        elif os.path.isfile(globbedpath):
+            files.append(globbedpath)
+    return files
+
+def execute_program(args):
+    """
+    Execute a process and wait for it to complete.  Returns all
+    output (stderr and stdout).
+
+    @param args is passed into subprocess.Popen().
+
+    @returns a tuple (returncode, output)
+    """

[rmistry, 2014/04/28 16:58:44]

"""Executes a process and waits for it to complete.

@param args is passed into subprocess.Popen().
@returns a tuple of the process output (returncode, output)
"""

[robertphillips, 2014/04/28 17:45:58]

Done.

+    proc = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)

[rmistry, 2014/04/28 16:58:44]

Nit: max line length in python is 80.

[robertphillips, 2014/04/28 17:45:58]

Done.

+    output, ignore = proc.communicate()
+    errcode = proc.returncode
+    return (errcode, output)
+
+class TestObj(object):
+
+    def __init__(self):
+        self.bench_pictures = find_path_to_program('bench_pictures')
+        self.gpuveto = find_path_to_program('gpuveto')
+        assert os.path.isfile(self.bench_pictures)
+        assert os.path.isfile(self.gpuveto)
+        self.truePositives = 0
+        self.falsePositives = 0
+        self.trueNegatives = 0
+        self.falseNegatives = 0
+
+    def process_skps(self, dir_or_file):
+        for skp in enumerate(dir_or_file):
+            self.process_skp(skp[1])
+
+        sys.stdout.write('TP %d FP %d TN %d FN %d\n' % (self.truePositives,
+                                            self.falsePositives,
+                                            self.trueNegatives,
+                                            self.falseNegatives))
+
+
+    def process_skp(self, skp_file):
+        assert os.path.isfile(skp_file)
+
+        # run gpuveto on the skp
+        args = [self.gpuveto, '-r', skp_file]
+        returncode, output = execute_program(args)
+        if (returncode != 0):
+            return
+
+        if ('unsuitable' in output):
+            suitable = False
+        else:
+            assert 'suitable' in output
+            suitable = True
+
+        # run raster config
+        args = [self.bench_pictures, '-r', skp_file, '--repeat', '20', '--config', '8888']

[rmistry, 2014/04/28 16:58:44]

Nit: max line length in python is 80.

[robertphillips, 2014/04/28 17:45:58]

Done.

+        returncode, output = execute_program(args)
+        if (returncode != 0):
+            return
+
+        matches = re.findall('[\d.]+', output)
+        if len(matches) != 4:
+            return
+
+        rasterTime = float(matches[3])
+
+        # run gpu config
+        args2 = [self.bench_pictures, '-r', skp_file, '--repeat', '20', '--config', 'gpu']

[rmistry, 2014/04/28 16:58:44]

Nit: max line length in python is 80.

[robertphillips, 2014/04/28 17:45:58]

Done.

+        returncode, output = execute_program(args2)
+        if (returncode != 0):
+            return
+
+        matches = re.findall('[\d.]+', output)
+        if len(matches) != 4:
+            return
+
+        gpuTime = float(matches[3])
+
+        sys.stdout.write("%s: gpuveto: %d raster %.2f gpu: %.2f\n" % (skp_file,

[rmistry, 2014/04/28 16:58:44]

Move all arguments to the next line:

sys.stdout.write("%s: gpuveto: %d raster %.2f gpu: %.2f\n" % (
    skp_file, suitable, rasterTime, gpuTime))

[robertphillips, 2014/04/28 17:45:58]

Done.

+                                                                      suitable,
+                                                                      rasterTime,
+                                                                      gpuTime))
+
+        if suitable:
+            if gpuTime < rasterTime:
+                self.truePositives += 1
+            else:
+                self.falsePositives += 1
+        else:
+            if gpuTime < rasterTime:
+                self.falseNegatives += 1
+            else:
+                self.trueNegatives += 1
+
+def main(main_argv):
+    if not main_argv or main_argv[0] in ['-h', '-?', '-help', '--help']:

[rmistry, 2014/04/28 16:58:44]

Could you please use argparse instead for this? It will make it more extensible
in the future.
Eg:

parser = argparse.ArgumentParser()
parser.add_argument('--skp_path',
                    help='Path to the SKP(s). Can either be a directory
containing SKPs or a single SKP.',
                    required=True)

args = parser.parse_args()
TestObj().process_skps(list_files(args.skp_path))
                    

+        sys.stderr.write(USAGE.format(command=__file__))
+        return 1
+    TestObj().process_skps(list_files(main_argv))
+
+if __name__ == '__main__':
+    exit(main(sys.argv[1]))

[rmistry, 2014/04/28 16:58:44]

Please use sys.exit(main(sys.argv[1])) instead.

[robertphillips, 2014/04/28 17:45:58]

Done.