Add benchmark suite for shortest path algorithms on weighted graphs#8059
Add benchmark suite for shortest path algorithms on weighted graphs#8059MridulS merged 9 commits intonetworkx:mainfrom
Conversation
rossbar
left a comment
There was a problem hiding this comment.
I'm personally not a huge fan of the data class indirection - why not just add the benchmark cases explicitly? IMO this is harder to follow!
The current approach has the problem of loading all graphs in memory, even if you are not running that benchmark. So for example, if you have a big graph, then all benchmarks have to load it into memory. Indirection allows you to not have to hardcode the name for the benchmark. That is particularly useful when you want to use many different graphs for benchmarking. So for example, you just need to pass the function+args and then |
|
Oops, posted on wrong PR, sorry for the noise :) See #8059 |
aa5a13f to
ccccfc1
Compare
|
@rossbar friendly ping! Are you good here? |
a25405f to
9c0d413
Compare
|
I was able to construct a complete graph that produces relaxations for each edge when running Dijkstra from node 0. This case puts a lot of pressure on the heap and forces a path/predecesor recomputation for each discovered edge. For such a graph of 1000 nodes, previous version of |
dschult
left a comment
There was a problem hiding this comment.
I like the new weighted complete graph with lots of work for Dijkstra. :) nice construction of weights!
The changes in code to shortest_path seem to consist of three changes:
- storing dict values to avoid double lookups
- using
getinstead ofin+subscript - using
setdefaultin one place
Can we tease these apart? All cases of 1) are clearly good. Cases of 2 are arguably less readable. Case of 3 is perhaps not even needed (or helpful, see comment below).
The relative timing of these changes with versions of Python. So as Python has worked to speed up function calls, get has become more competitive against in+subscript. But that may change. The readability of if u in dist: is hard to beat. So I'm hoping that our speedup are mostly from 1) and somewhat from 2) and maybe not at all from 3). :)
|
Oh wait I added the Dijkstra's change by mistake! This PR was only intended to have the benchmark changes. The speed up I was referring to is from the already merged PR on bidirectional Dijkstra. |
|
Fixed! I reverted uninteded changes |
|
@rossbar I changed the worst case graph generation. You can see how it generates the worst case where all edges produce a new relaxation (#8218 (comment)). Let me know if you have other comments on the PR! |
|
Friendly ping! It would be great to merge this one so we track the benchmarks used for shortest paths algos |
MridulS
left a comment
There was a problem hiding this comment.
LGTM, I like the dijkstra construction! Maybe we can make it directed to be even more "strict"? But good to go for me :)
This PR introduces a new benchmarking suite for evaluating shortest path algorithms on weighted graphs. This benchmark was used to evaluate #8023. It includes the following changes:
Graphs added
WeightedGraphBenchmarkclass:networkx.single_source_dijkstraon various weighted graphs.path_graph(sizes: 1,000–20,000) anderdos_renyi_graph(sizes: 10, 100, 1000; probabilities: 0.1, 0.5, 0.9).Utilities added
benchmarks/utils.py:weighted_graph: Utility to assign random integer weights to edges.benchmark_name_from_func_call: Generates human-readable graph names from function calls for better benchmark output labeling.Example