debugpythonModerate
Executing a program with a temporary directory and measuring the running time
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.
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.