Optimize diameter calculation for weighted graphs#495
Optimize diameter calculation for weighted graphs#495LoveLow-Global wants to merge 11 commits intoJuliaGraphs:masterfrom
Conversation
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #495 +/- ##
==========================================
+ Coverage 97.29% 97.32% +0.02%
==========================================
Files 126 126
Lines 7662 7726 +64
==========================================
+ Hits 7455 7519 +64
Misses 207 207 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
We now have a benchmarks file and as you can see it is executed in each PR. Could you add a (fairly small, fast) benchmark to that file? Both of the slow approach and of the new approach (I understand the API for the new approach would be the "slow approach" until this is merged)? |
|
Hello, thanks for the feedback. I am not sure if I have understood your feedback, but I have moved the benchmarking part in |
|
One more thing and this is probably good to go. I poorly understood how you had structured the tests. It is good that you have added the benchmarks, they are useful and this is a good place for you. But in fulfilling my poorly phrased request, I think you have completely removed any tests for this algorithm on weighted graphs. Could you, in addition to the benchmarks, also add correctness tests to the test suite, even if code-wise they are very similar. |
|
Oh, and I started keeping a CHANGELOG -- could you add an entry in it as well? |
|
I have edited the codes and added a line on CHANGELOG, I hope this looks okay. Also, if Graphs.jl now uses a benchmark section, I believe changes to lines 83-139 in |
|
I used Opus 4.6 to generate a bunch of edge-case tests and I think it found a broken edge case. The prompt was this: "In Graphs.jl PR 495 a new algorithm is being implemented. Review it, especially focusing on edge cases. Locally generate and run additional tests in order to try to detect bugs, including commond graphs, random graphs, graphs with self-loops, empty graphs, and other edge cases." Could you please check the PR towards your own branch (LoveLow-Global#1) with the suggested new tests and (in a separate commit) the potentially wrong edge-case fix. I do not particularly trust Opus to do this correctly, but I am stretched pretty thin and can not commit to doing a more detailed review (I apologize for this review asymmetry -- I know it is frustrating but we simply do not have enough reviewers right now). I think the bug that Opus found is real, I am not at all trusting of its potential fix. Speaking of which, given the struggles Graphs.jl has with getting enough maintainers, and your already significant contributions, do let me know if you are interested in helping with the occasional code review -- even once or twice a year can be very valuable for the community. Thanks again for your submission! |
|
Also, to clarify -- no need to accept the PR from Opus at all! This is your code and it is much better than what Opus does. The PR was created just so that you can see the tests for yourself and what is the failing one and what is the guessed source of the failure. |
|
Thanks for the comment and the PR, I will try to investigate on the BUG on |
|
No worries,we are all volunteers here! |
2 participants
This edit will be the extension of the pull request about two months ago. At the time, the iFUB algorithm was only applied to unweighted graphs, but now it is implemented to weighted graphs as well.
To compare with the original method of using
diameter(eccentricities::Vector) = maximum(eccentricities), I have added a testset called"Weighted Diameter Benchmarks"undertest/distance.jl. This should be removed before merging.According to the Benchmarks mentioned above, this implementation makes diameter calculation of real-world like graphs (when tested on Barabási-Albert Model) over 50 times faster, and diameter calculation on random graph models (when tested on Erdős-Rényi Graph) over 2 times faster. This happens because the algorithm is more useful when some vertices have very high degrees.