Skip to content
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

Merged
merged 2 commits into from
Mar 3, 2021

Conversation

jwiggins
Copy link
Member

@jwiggins jwiggins commented Mar 2, 2021

Resolves #650

Screen Shot 2021-03-02 at 5 12 45 PM

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.

@jwiggins
Copy link
Member Author

jwiggins commented Mar 2, 2021

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}
Copy link
Contributor

@aaronayres35 aaronayres35 Mar 2, 2021

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]
Copy link
Contributor

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

Copy link
Contributor

@aaronayres35 aaronayres35 left a 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

@jwiggins
Copy link
Member Author

jwiggins commented Mar 3, 2021

The code is a little thick to read through and names might help make it slightly more understandable.

That's being charitable. I'll take a quick shot at some cleanup this morning.

Thanks for the review/testing.

@jwiggins jwiggins merged commit 42dc7aa into master Mar 3, 2021
@jwiggins jwiggins deleted the feature/benchmark-file-backends branch March 3, 2021 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kiva Benchmark: Include file-based backends
2 participants