Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[WIP] Benchmark main ArviZ code #1088

Closed
wants to merge 5 commits into from
Closed

[WIP] Benchmark main ArviZ code #1088

wants to merge 5 commits into from

Conversation

OriolAbril
Copy link
Member

Description

Benchmarks currently have a copied version of ArviZ's functions in their code instead of using ArviZ itself. Once finished it will fix #782.

Moving the benchmarks one folder below allows to import ArviZ. Then, to ease comparison, it would be great to use asv parametrization options, so that both numba and non-numba version are run in the same environment.

Checklist

  • Follows official PR format
  • New features are properly documented (with an example if appropriate)?
  • Code style correct (follows pylint and black guidelines) -> should benchmarks comply with lint too?
  • Changes are listed in changelog

@ahartikainen
Copy link
Contributor

What different things we want to test?

All diagnostics / stats functions?

Also, should we implement LineProfiler for all the functions?

Also maybe a viz about our codebase?

https://pycallgraph.readthedocs.io/en/master/
https://stackoverflow.com/questions/582336/how-can-you-profile-a-python-script

And then add one file that does some processing for the results?

@OriolAbril
Copy link
Member Author

I would start (small) with asv benchmarks timing some common functions. Also, if they have numba option, compare between numba and no numba. This will warn us when there is a suspicious change in the code.

One example is _fast_kde which currently has no numba speedup whatsoever since it was moved to plot_utils. In the moving changed the import from stats_utils.histogram to np.histogram. (trying to fix this causes the cyclic import, we should move fast_kdes to their own file or maybe to stats_utils)

Once we are happy with these benchmarks we can try to go deeper, check memory usage, line profiling...

@codecov
Copy link

codecov bot commented Mar 20, 2020

Codecov Report

Merging #1088 into master will increase coverage by 0.77%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1088      +/-   ##
==========================================
+ Coverage   92.68%   93.45%   +0.77%     
==========================================
  Files          93       93              
  Lines        9032     8705     -327     
==========================================
- Hits         8371     8135     -236     
+ Misses        661      570      -91
Impacted Files Coverage Δ
arviz/plots/backends/bokeh/jointplot.py 92.68% <0%> (-5.32%) ⬇️
arviz/plots/forestplot.py 90% <0%> (-4.12%) ⬇️
arviz/plots/backends/bokeh/hpdplot.py 96% <0%> (-4%) ⬇️
arviz/plots/backends/bokeh/loopitplot.py 85.29% <0%> (-3.6%) ⬇️
arviz/plots/backends/bokeh/parallelplot.py 96.66% <0%> (-3.34%) ⬇️
arviz/plots/backends/bokeh/violinplot.py 91.66% <0%> (-3.08%) ⬇️
arviz/plots/backends/bokeh/mcseplot.py 95.58% <0%> (-2.88%) ⬇️
arviz/plots/backends/bokeh/essplot.py 95.65% <0%> (-2.84%) ⬇️
arviz/plots/backends/bokeh/rankplot.py 88.88% <0%> (-2.78%) ⬇️
arviz/plots/backends/bokeh/autocorrplot.py 94.59% <0%> (-2.55%) ⬇️
... and 44 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1ccd0d...aab1c45. Read the comment docs.

@nitishp25
Copy link
Contributor

Can't we import histogram, stats_variance_1d and stats_variance_2d functions as well from az.stats.stats_utils and make the benchmarks using Numba params like you have done for the rest of the functions?

@OriolAbril
Copy link
Member Author

I think there are some issues due to cyclic imports. We have to check functions in stats utils and plot utils to make sure that plot utils imports stats utils and stats utils does not import plot utils.

@OriolAbril
Copy link
Member Author

@nitishp25 also feel free to check out this branch locally to build on top of this and submit the work in a different PR

@nitishp25 nitishp25 mentioned this pull request Apr 5, 2020
4 tasks
@OriolAbril
Copy link
Member Author

I'll close in favour of #1142

@OriolAbril OriolAbril closed this Apr 7, 2020
@OriolAbril OriolAbril deleted the benchmarks branch May 24, 2020 14:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A better way to benchmark the speed tests
3 participants