improve method cycle_basis for graphs with multiple edges
#36493
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kwankyu
reviewed
Oct 24, 2023
| [(4, 3, 'd'), (3, 2, 'b'), (2, 1, 'a'), (1, 4, 'f')], | ||
| [(4, 3, 'e'), (3, 2, 'b'), (2, 1, 'a'), (1, 4, 'f')]] | ||
| [(3, 4, 'd'), (4, 1, 'f'), (1, 2, 'a'), (2, 3, 'b')], | ||
| [(3, 4, 'e'), (4, 1, 'f'), (1, 2, 'a'), (2, 3, 'b')]] | ||
| """ |
There was a problem hiding this comment.
Then for these cases, you may remove # needs networkx?
|
Looks good to me though I don't know the algorithm. Can you provide some benchmarks showing that the new algorithm is more efficient? |
|
Some benchmarks on the graphs used in the doctests: sage: G = Graph([(0, 2, 'a'), (0, 2, 'b'), (0, 1, 'c'), (1, 2, 'd')], multiedges=True)
sage: H = Graph([(1, 2), (2, 3), (2, 3), (3, 4), (1, 4), (1, 4), (4, 5), (5, 6), (4, 6), (6, 7)], multiedges=True)Before: sage: %timeit G.cycle_basis()
57.3 µs ± 177 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: %timeit H.cycle_basis()
112 µs ± 190 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)After: sage: %timeit G.cycle_basis()
42.5 µs ± 104 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
sage: %timeit H.cycle_basis()
56.8 µs ± 176 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)and a larger graph sage: G = graphs.Grid2dGraph(10, 10)
sage: G.allow_multiple_edges(True)
sage: G.add_edges(G.edges())Before sage: %timeit G.cycle_basis()
48.1 ms ± 359 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)After sage: %timeit G.cycle_basis()
1.13 ms ± 4.2 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each) |
|
Documentation preview for this PR (built with commit 00c5af4; changes) is ready! 🎉 |
|
If you want to attract attention from graph theory experts for more time, you may discard my review. |
|
Thank you. |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 26, 2023
sagemathgh-36493: improve method `cycle_basis` for graphs with multiple edges We use the notion of nearest common ancestor in a tree to find cycles more efficiently than using DFS. This is used only when the graph allows multiple edges, since otherwise we use the method of networkx. Therefore, the failing doctests reported in sagemath#36486 when using networkx 3.2 are independent from the changes done here. An additional optimization would be to determine the minimum spanning tree, the ranks and the parents at the same time. We don't do it here. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36493 Reported by: David Coudert Reviewer(s): Kwankyu Lee
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 28, 2023
sagemathgh-36493: improve method `cycle_basis` for graphs with multiple edges We use the notion of nearest common ancestor in a tree to find cycles more efficiently than using DFS. This is used only when the graph allows multiple edges, since otherwise we use the method of networkx. Therefore, the failing doctests reported in sagemath#36486 when using networkx 3.2 are independent from the changes done here. An additional optimization would be to determine the minimum spanning tree, the ranks and the parents at the same time. We don't do it here. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36493 Reported by: David Coudert Reviewer(s): Kwankyu Lee
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 28, 2023
sagemathgh-36493: improve method `cycle_basis` for graphs with multiple edges We use the notion of nearest common ancestor in a tree to find cycles more efficiently than using DFS. This is used only when the graph allows multiple edges, since otherwise we use the method of networkx. Therefore, the failing doctests reported in sagemath#36486 when using networkx 3.2 are independent from the changes done here. An additional optimization would be to determine the minimum spanning tree, the ranks and the parents at the same time. We don't do it here. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36493 Reported by: David Coudert Reviewer(s): Kwankyu Lee
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 29, 2023
sagemathgh-36493: improve method `cycle_basis` for graphs with multiple edges We use the notion of nearest common ancestor in a tree to find cycles more efficiently than using DFS. This is used only when the graph allows multiple edges, since otherwise we use the method of networkx. Therefore, the failing doctests reported in sagemath#36486 when using networkx 3.2 are independent from the changes done here. An additional optimization would be to determine the minimum spanning tree, the ranks and the parents at the same time. We don't do it here. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36493 Reported by: David Coudert Reviewer(s): Kwankyu Lee
|
Conda workflow fails because of this PR: https://github.com/sagemath/sage/actions/runs/6700323341/job/18206090258#step:11:10801 (with python 3.9) |
|
Sorry, I did not realized that pairwise was added to itertools version 3.10. Should I propose a fix to avoid using pairwise ? |
|
No problem 🐱 . Yes, a follow-up compatible with 3.9 would be good (since its still a bit until we can talk about dropping this version). |
5 tasks
|
This is fixed in #36609 |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Nov 2, 2023
sagemathgh-36609: avoid using `itertools.pairwise` Method `pairwise` has been introduced in `itertools` version 3.10. As reported in sagemath#36493, its use breaks the condo workflow with Python 3.9. So we modify the code to avoid using `pairwise`. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies See discussion sagemath#36493 (comment), URL: sagemath#36609 Reported by: David Coudert Reviewer(s): Tobias Diez
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Nov 5, 2023
sagemathgh-36609: avoid using `itertools.pairwise` Method `pairwise` has been introduced in `itertools` version 3.10. As reported in sagemath#36493, its use breaks the condo workflow with Python 3.9. So we modify the code to avoid using `pairwise`. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies See discussion sagemath#36493 (comment), URL: sagemath#36609 Reported by: David Coudert Reviewer(s): Tobias Diez
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We use the notion of nearest common ancestor in a tree to find cycles more efficiently than using DFS.
This is used only when the graph allows multiple edges, since otherwise we use the method of networkx. Therefore, the failing doctests reported in #36486 when using networkx 3.2 are independent from the changes done here.
An additional optimization would be to determine the minimum spanning tree, the ranks and the parents at the same time. We don't do it here.
📝 Checklist
⌛ Dependencies