HiveBrain v1.2.0
Get Started
← Back to all entries
debugpythonModerate

Executing a program with a temporary directory and measuring the running time

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
executingdirectorythetemporarywithprogramtimerunningmeasuringand

Problem

I need a code review for best practices and code correctness of the following piece of code. run executes a program and validates in output. If the output is valid status can be found in VALID_SOLUTIONS.

elapsed = None
tmpdir = tempfile.mkdtemp(prefix='structure-%s-' % os.path.basename(instance))
try:
  for r in range(options.repeat):
    run_returncode, run_status, run_elapsed = run(
        r, path, program, tmpdir, options.timeout, options.satisfiable)
    if run_status not in VALID_SOLUTIONS:
      returncode, status, elapsed = run_returncode, run_status, None
      break

    if r == 0:
      # Set the expected results
      returncode, status = run_returncode, run_status
      elapsed = [run_elapsed]
    else:
      if returncode != run_returncode or status != run_status:
        # Results don't match
        returncode, status, elapsed = None, 'inconsistent_results', None
        break
      else:
        # Extra set
        elapsed.append(run_elapsed)
except Exception as e:
  # Unexpected error in script itself
  print(e, file=sys.stderr)
  #traceback.print_exc()
  status = 'error'
except KeyboardInterrupt as e:
  print('interrupted', file=sys.stderr)
  returncode, status, elapsed = None, 'interrupted', None
finally:
  if status in VALID_SOLUTIONS:
    avg = sum(elapsed) / len(elapsed)
    std = (sum((e - avg)**2 for e in elapsed) / len(elapsed)) ** 0.5
    ci = 1.96 * std / (len(elapsed) ** 0.5)
    print(instance, returncode, status, avg, ci)
  else:
    print(instance, returncode, status, None, None)

  sys.stdout.flush()
  exit(int(status not in ['interrupted'] + VALID_SOLUTIONS))

Solution

Well, firstly, don't be afraid of empty lines. This code is quite hard to read.

Secondly, functions is good invention. ;) That code as it is now is a bare piece of code, with no indication of where most of the parameters come from, and it seems that the acceptable solutions is a global, as you have capitalized it? That doesn't make much sense to me.

Just as you have a function called run() to run the program, it would make sense to have a function to run it multiple times, with the tests for that you have.

I'm also not sure why you trap KeyboardInterrupt, without seeming to actually do anything with it, except setting the status, and then testing the status. You could just do exit directly from that exception handler, that would be clearer.

Otherwise it seems fine. Feedback on correctness is impossible when your code isn't a running example. But then again, tests generally work better than code reviews there, although they are not exclusive.

Context

StackExchange Code Review Q#519, answer score: 12

Revisions (0)

No revisions yet.