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

faster bidirectional dijkstra using pairing heap #39228

Conversation

dcoudert
Copy link
Contributor

@dcoudert dcoudert commented Dec 30, 2024

We improve methods bidirectional_dijkstra and bidirectional_dijkstra_special from c_graph.py by using the new pairing heap data structure (see #39046) instead of a priority_queue.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@dcoudert
Copy link
Contributor Author

a few timings to see the advantage of the new implementation

sage: G = graphs.PetersenGraph()
sage: %timeit G._backend.bidirectional_dijkstra_old(0, 1)
10.6 µs ± 31.9 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
sage: %timeit G._backend.bidirectional_dijkstra(0, 1)
10.4 µs ± 10.4 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
sage: G = graphs.RandomGNP(10, .5)
sage: %timeit G._backend.bidirectional_dijkstra_old(0, 1)
23.2 µs ± 65 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: %timeit G._backend.bidirectional_dijkstra(0, 1)
20.6 µs ± 105 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: G = graphs.RandomGNP(10, .3)
sage: %timeit G._backend.bidirectional_dijkstra_old(0, 1)
17.2 µs ± 41.8 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
sage: %timeit G._backend.bidirectional_dijkstra(0, 1)
15.6 µs ± 41.6 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
sage: G = graphs.RandomGNP(100, .05)
sage: %timeit G._backend.bidirectional_dijkstra_old(0, 1)
309 µs ± 908 ns per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
sage: %timeit G._backend.bidirectional_dijkstra(0, 1)
259 µs ± 677 ns per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
sage: G = graphs.RandomGNP(100, .1)
sage: %timeit G._backend.bidirectional_dijkstra_old(0, 1)
587 µs ± 1.48 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
sage: %timeit G._backend.bidirectional_dijkstra(0, 1)
437 µs ± 1.2 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
sage: G = graphs.RandomGNP(100, .3)
sage: %timeit G._backend.bidirectional_dijkstra_old(0, 1)
1.61 ms ± 3.18 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
sage: %timeit G._backend.bidirectional_dijkstra(0, 1)
1.2 ms ± 1.67 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
sage: G = graphs.RandomGNP(10000, .0005)
sage: %timeit G._backend.bidirectional_dijkstra_old(0, 1)
35.6 ms ± 170 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: %timeit G._backend.bidirectional_dijkstra(0, 1)
31.7 ms ± 311 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: G = graphs.RandomGNP(10000, .001)
sage: %timeit G._backend.bidirectional_dijkstra_old(0, 1)
61.8 ms ± 137 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: %timeit G._backend.bidirectional_dijkstra(0, 1)
52.5 ms ± 615 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: G = graphs.RandomGNP(10000, .005)
sage: %timeit G._backend.bidirectional_dijkstra_old(0, 1)
119 ms ± 542 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
sage: %timeit G._backend.bidirectional_dijkstra(0, 1)
103 ms ± 1 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

@dcoudert dcoudert self-assigned this Dec 30, 2024
@dcoudert dcoudert requested a review from gmou3 December 30, 2024 16:54
Copy link

github-actions bot commented Dec 30, 2024

Documentation preview for this PR (built with commit 3962a93; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@gmou3
Copy link
Contributor

gmou3 commented Jan 10, 2025

It does seem to be faster indeed. Is there a theoretical guarantee for this?

Can you remove the _old method so we can review the code changes more easily?

@dcoudert
Copy link
Contributor Author

dcoudert commented Jan 10, 2025

With a priority queue, insertion is done in $O(\log{n})$-time and extraction of the top item (a max) in $O(\log{n})$-time. Consequently, the overall time complexity of the algorithm was in $O((n + m)\log{n})$.

With a pairing heap, insertion is done in $O(1)$-time and extraction of the top item (a min) in $O(\log{n})$-time. Consequently, the overall time complexity of the algorithm is in $O(m + n\log{n})$. This is the correct way to implement the Dijkstra algorithm.

Copy link
Contributor

@gmou3 gmou3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. What about replacing the priority queue with a pairing heap everywhere in the file?

@dcoudert
Copy link
Contributor Author

I did the changes in bidirectional_dijkstra_special and updated the description of the PR accordingly.

Copy link
Contributor

@gmou3 gmou3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@dcoudert
Copy link
Contributor Author

Thanks for the review.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 12, 2025
    
We improve methods `bidirectional_dijkstra` and
`bidirectional_dijkstra_special` from `c_graph.py` by using the new
pairing heap data structure (see sagemath#39046) instead of a `priority_queue`.


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39228
Reported by: David Coudert
Reviewer(s): David Coudert, gmou3
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 16, 2025
    
We improve methods `bidirectional_dijkstra` and
`bidirectional_dijkstra_special` from `c_graph.py` by using the new
pairing heap data structure (see sagemath#39046) instead of a `priority_queue`.


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39228
Reported by: David Coudert
Reviewer(s): David Coudert, gmou3
@vbraun vbraun merged commit 9f4042a into sagemath:develop Jan 18, 2025
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants