-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run the kiva benchmark on all backends #666
Conversation
I've got the PDF backend partially working now. We can clean up the remaining red X's elsewhere. |
print(" ... Not available") | ||
continue | ||
results[name] = benchmark_backend(suite, name, module, outdir=outdir) | ||
results = {t: {} for t in _BACKENDS} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could 't' be 'backend_type` or 'btype' here like below?
# Which gets used to add a CSS class for styling | ||
klass = "valid" if valid else "invalid" | ||
# Each backend stat includes a CSS class for table styling | ||
stat, klass = stats[bend] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this stats could be called something different in the for loop above? It is the stats after they have been run though a formatting function eg _format_benchmark
.
It is pretty inconsequential, but the code names are a little confusing as was thinking stats
was a dict of a different form at first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested locally and everything looks correct. The links to pdf and ps worked as well
Just a couple comments about some variable names. The code is a little thick to read through and names might help make it slightly more understandable. Also some numpy docstrings giving more details about parameters for the major functions could be very useful, but feel free to ignore at least for this PR. That can be done as some cleanup later if we find the time
That's being charitable. I'll take a quick shot at some cleanup this morning. Thanks for the review/testing. |
Resolves #650
As you can see, there's something wrong with the PDF backend. That can be fixed elsewhere. Additionally, the code for writing the HTML output is getting really awful. It could benefit from some clean up if we think we might add any more enhancements.