-
-
Notifications
You must be signed in to change notification settings - Fork 487
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
graphs: add implementation of slice decomposition via an extended LexBFS algorithm #38299
graphs: add implementation of slice decomposition via an extended LexBFS algorithm #38299
Conversation
Documentation preview for this PR (built with commit 0d26f30; changes) is ready! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not easy to review (large PR) but I like it. Thank you. I have a few minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like what you did in traversals.pyx
.
I just have a few more minor remarks.
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I found a bug in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. LGTM.
…ia an extended LexBFS algorithm <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> This PR implements a new graph decomposition: slice decomposition. Precise definition and a description of the algorithm can be found in [TCHP2008 (section 3.2 & 3.3)](https://arxiv.org/pdf/0710.3901v3). I implemented a new method (only for undirected graphs) called `slice_decomposition`. It returns a new class `SliceDecompostion`, with methods to retrieve the information about the computed slice decomposition. The code is in a new file: `graph_decompositions/slice_decomposition.pyx`. The actual computation is done with a new function called `extended_lex_BFS` which only works for undirected graphs. It is very close to the previous `lex_BFS_fast_short_digraph` from `traversals.pyx`, the difference being that the new function works with the `CGraph` directly (no conversion needed to `short_digraph` anymore) and it computes more information (lexicographic labels and length of x-slices). The complexity of the two algorithms are the same. So I removed `lex_BFS_fast_short_digraph` from `traversals.pyx` and used `extended_lex_BFS` instead for the "fast" version of the `lex_BFS` method. I took the opportunity to factorize the code between `lex_BFS` ("slow" version), `lex_DFS`, `lex_UP` and `lex_DOWN` in `traversals.pyx`: there is no more conversion to `short_digraph` and they all call the same common function (remark: i was not able to put the doc in common, so there is still some repetition between the doc of these methods) ### 📝 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. - [x] I have created tests covering the changes. - [x] 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#38299 Reported by: cyrilbouvier Reviewer(s): cyrilbouvier, David Coudert
On 32-bit:
|
Should be fixed by 0d26f30 |
sage: P3 = graphs.PathGraph(3) | ||
sage: SD1 = P3.slice_decomposition(initial_vertex=0) | ||
sage: SD2 = P3.slice_decomposition(initial_vertex=2) | ||
sage: len({SD1: 1, SD2: 2}) # indirect doctest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems ok, but why not simply hash(SD1) != hash(SD2)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got the idea by looking at other doctests for other __hash__
methods.
By thinking a little about it, there is a nonzero probability that there exists an architecture where the hashes could be equal. But as keys of a dict are compare via __eq__
if they have the same hashes, even in this (very very) improbable case the test should pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Thank you for the clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
…ia an extended LexBFS algorithm <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> This PR implements a new graph decomposition: slice decomposition. Precise definition and a description of the algorithm can be found in [TCHP2008 (section 3.2 & 3.3)](https://arxiv.org/pdf/0710.3901v3). I implemented a new method (only for undirected graphs) called `slice_decomposition`. It returns a new class `SliceDecompostion`, with methods to retrieve the information about the computed slice decomposition. The code is in a new file: `graph_decompositions/slice_decomposition.pyx`. The actual computation is done with a new function called `extended_lex_BFS` which only works for undirected graphs. It is very close to the previous `lex_BFS_fast_short_digraph` from `traversals.pyx`, the difference being that the new function works with the `CGraph` directly (no conversion needed to `short_digraph` anymore) and it computes more information (lexicographic labels and length of x-slices). The complexity of the two algorithms are the same. So I removed `lex_BFS_fast_short_digraph` from `traversals.pyx` and used `extended_lex_BFS` instead for the "fast" version of the `lex_BFS` method. I took the opportunity to factorize the code between `lex_BFS` ("slow" version), `lex_DFS`, `lex_UP` and `lex_DOWN` in `traversals.pyx`: there is no more conversion to `short_digraph` and they all call the same common function (remark: i was not able to put the doc in common, so there is still some repetition between the doc of these methods) ### 📝 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. - [x] I have created tests covering the changes. - [x] 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#38299 Reported by: cyrilbouvier Reviewer(s): cyrilbouvier, David Coudert
…ia an extended LexBFS algorithm <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> This PR implements a new graph decomposition: slice decomposition. Precise definition and a description of the algorithm can be found in [TCHP2008 (section 3.2 & 3.3)](https://arxiv.org/pdf/0710.3901v3). I implemented a new method (only for undirected graphs) called `slice_decomposition`. It returns a new class `SliceDecompostion`, with methods to retrieve the information about the computed slice decomposition. The code is in a new file: `graph_decompositions/slice_decomposition.pyx`. The actual computation is done with a new function called `extended_lex_BFS` which only works for undirected graphs. It is very close to the previous `lex_BFS_fast_short_digraph` from `traversals.pyx`, the difference being that the new function works with the `CGraph` directly (no conversion needed to `short_digraph` anymore) and it computes more information (lexicographic labels and length of x-slices). The complexity of the two algorithms are the same. So I removed `lex_BFS_fast_short_digraph` from `traversals.pyx` and used `extended_lex_BFS` instead for the "fast" version of the `lex_BFS` method. I took the opportunity to factorize the code between `lex_BFS` ("slow" version), `lex_DFS`, `lex_UP` and `lex_DOWN` in `traversals.pyx`: there is no more conversion to `short_digraph` and they all call the same common function (remark: i was not able to put the doc in common, so there is still some repetition between the doc of these methods) ### 📝 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. - [x] I have created tests covering the changes. - [x] 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#38299 Reported by: cyrilbouvier Reviewer(s): cyrilbouvier, David Coudert
…ia an extended LexBFS algorithm <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> This PR implements a new graph decomposition: slice decomposition. Precise definition and a description of the algorithm can be found in [TCHP2008 (section 3.2 & 3.3)](https://arxiv.org/pdf/0710.3901v3). I implemented a new method (only for undirected graphs) called `slice_decomposition`. It returns a new class `SliceDecompostion`, with methods to retrieve the information about the computed slice decomposition. The code is in a new file: `graph_decompositions/slice_decomposition.pyx`. The actual computation is done with a new function called `extended_lex_BFS` which only works for undirected graphs. It is very close to the previous `lex_BFS_fast_short_digraph` from `traversals.pyx`, the difference being that the new function works with the `CGraph` directly (no conversion needed to `short_digraph` anymore) and it computes more information (lexicographic labels and length of x-slices). The complexity of the two algorithms are the same. So I removed `lex_BFS_fast_short_digraph` from `traversals.pyx` and used `extended_lex_BFS` instead for the "fast" version of the `lex_BFS` method. I took the opportunity to factorize the code between `lex_BFS` ("slow" version), `lex_DFS`, `lex_UP` and `lex_DOWN` in `traversals.pyx`: there is no more conversion to `short_digraph` and they all call the same common function (remark: i was not able to put the doc in common, so there is still some repetition between the doc of these methods) ### 📝 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. - [x] I have created tests covering the changes. - [x] 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#38299 Reported by: cyrilbouvier Reviewer(s): cyrilbouvier, David Coudert
This PR implements a new graph decomposition: slice decomposition. Precise definition and a description of the algorithm can be found in TCHP2008 (section 3.2 & 3.3).
I implemented a new method (only for undirected graphs) called
slice_decomposition
. It returns a new classSliceDecompostion
, with methods to retrieve the information about the computed slice decomposition.The code is in a new file:
graph_decompositions/slice_decomposition.pyx
.The actual computation is done with a new function called
extended_lex_BFS
which only works for undirected graphs. It is very close to the previouslex_BFS_fast_short_digraph
fromtraversals.pyx
, the difference being that the new function works with theCGraph
directly (no conversion needed toshort_digraph
anymore) and it computes more information (lexicographic labels and length of x-slices). The complexity of the two algorithms are the same.So I removed
lex_BFS_fast_short_digraph
fromtraversals.pyx
and usedextended_lex_BFS
instead for the "fast" version of thelex_BFS
method.I took the opportunity to factorize the code between
lex_BFS
("slow" version),lex_DFS
,lex_UP
andlex_DOWN
in
traversals.pyx
: there is no more conversion toshort_digraph
and they all call the same common function(remark: i was not able to put the doc in common, so there is still some repetition between the doc of these methods)
📝 Checklist
⌛ Dependencies