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.

Sign up for GitHub

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

Jump to bottom

Improve complexity comments for graphs #38198

Merged
merged 3 commits into from
Jun 22, 2024
Merged

Improve complexity comments for graphs #38198

merged 3 commits into from
Jun 22, 2024

Conversation

cyrilbouvier
Copy link
Contributor

As noted in #37642, there is a difference in the complexity of enumerating neighbors or edges between sparse graphs and dense graphs.
This PR aims at making the comments about the time complexity of algorithms clearer in regard of this difference.

Most of the changes I made in this PR are due to the fact that the function init_short_digraph have the following time complexity:

  • O(n+m) when the input graph has a sparse representation and sort_neighbors is False
  • O(n+m log(m)) when the input graph has a sparse representation and sort_neighbors is True
  • O(n^2) when the input graph has a dense representation and sort_neighbors is False
  • O(n^2 log(m)) when the input graph has a dense representation and sort_neighbors is True

with m the number of edges and n the number of vertices of the graph.

I spotted 5 additional complexity claims in the comments that I was not able to verify:

  1. for strong_orientations_iterator in orientations.py
  2. for yen_k_shortest_simple_paths in path_enumeration.pyx
  3. for feng_k_shortest_simple_paths in path_enumeration.pyx
  4. for edge_disjoint_spanning_trees in spanning_tree.pyx
    (there is loop over the sorted edges of the graphs, which should imply a
    complexity of at least n+m log(m) for sparse graphs and O(n^2 log(m)) for dense graphs
  5. for is_partial_cube in partial_cube.py
    (there is at least one enumeration of neighbors of a vertex (via calling contracted[root],
    line 318) so m should appear in the complexity)

📝 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

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 11, 2024 •
edited
Loading

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

dcoudert
dcoudert approved these changes Jun 12, 2024
View reviewed changes
Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

LGTM.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
sagemathgh-38198: Improve complexity comments for graphs
2678c06 
    
<!-- ^ 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". -->

As noted in sagemath#37642, there is a difference in the complexity of
enumerating neighbors or edges between sparse graphs and dense graphs.
This PR aims at making the comments about the time complexity of
algorithms clearer in regard of this difference.

Most of the changes I made in this PR are due to the fact that the
function `init_short_digraph` have the following time complexity:
- `O(n+m)` when the input graph has a sparse representation and
sort_neighbors is False
- `O(n+m log(m))` when the input graph has a sparse representation and
sort_neighbors is True
- `O(n^2)` when the input graph has a dense representation and
sort_neighbors is False
- `O(n^2 log(m))` when the input graph has a dense representation and
sort_neighbors is True

with `m` the number of edges and `n` the number of vertices of the
graph.


I spotted 5 additional complexity claims in the comments that I was not
able to verify:
1. for strong_orientations_iterator in orientations.py
2. for yen_k_shortest_simple_paths in path_enumeration.pyx
3. for feng_k_shortest_simple_paths in path_enumeration.pyx
4. for edge_disjoint_spanning_trees in spanning_tree.pyx
    (there is loop over the sorted edges of the graphs, which should
imply a
    complexity of at least `n+m log(m)` for sparse graphs and `O(n^2
log(m))`  for dense graphs
5. for is_partial_cube in partial_cube.py
    (there is at least one enumeration of neighbors of a vertex (via
calling `contracted[root]`,
    line 318) so `m` should appear in the complexity)



### 📝 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.
- [x] 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#38198
Reported by: cyrilbouvier
Reviewer(s): cyrilbouvier, David Coudert
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
sagemathgh-38198: Improve complexity comments for graphs
fd428c7 
    
<!-- ^ 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". -->

As noted in sagemath#37642, there is a difference in the complexity of
enumerating neighbors or edges between sparse graphs and dense graphs.
This PR aims at making the comments about the time complexity of
algorithms clearer in regard of this difference.

Most of the changes I made in this PR are due to the fact that the
function `init_short_digraph` have the following time complexity:
- `O(n+m)` when the input graph has a sparse representation and
sort_neighbors is False
- `O(n+m log(m))` when the input graph has a sparse representation and
sort_neighbors is True
- `O(n^2)` when the input graph has a dense representation and
sort_neighbors is False
- `O(n^2 log(m))` when the input graph has a dense representation and
sort_neighbors is True

with `m` the number of edges and `n` the number of vertices of the
graph.


I spotted 5 additional complexity claims in the comments that I was not
able to verify:
1. for strong_orientations_iterator in orientations.py
2. for yen_k_shortest_simple_paths in path_enumeration.pyx
3. for feng_k_shortest_simple_paths in path_enumeration.pyx
4. for edge_disjoint_spanning_trees in spanning_tree.pyx
    (there is loop over the sorted edges of the graphs, which should
imply a
    complexity of at least `n+m log(m)` for sparse graphs and `O(n^2
log(m))`  for dense graphs
5. for is_partial_cube in partial_cube.py
    (there is at least one enumeration of neighbors of a vertex (via
calling `contracted[root]`,
    line 318) so `m` should appear in the complexity)



### 📝 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.
- [x] 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#38198
Reported by: cyrilbouvier
Reviewer(s): cyrilbouvier, David Coudert
@vbraun vbraun merged commit 080cebe into sagemath:develop Jun 22, 2024
21 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Jul 8, 2024
@cyrilbouvier cyrilbouvier mentioned this pull request Jul 26, 2024
Closed
5 tasks
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 8, 2024
sagemathgh-38427: graphs: make init_short_digraph always sort neighbo…
c1fbf31 
…rs but without the extra log complexity

    
<!-- ^ 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 improve the `init_short_digraph` function that is used to
initialize `StaticSparseCGraph` (used for immutable `Graph` and
`DiGraph`).

Before, a boolean parameter `sort_neighbors` was used to specify if we
wanted to sort the neighbors or not. It implied an extra `log` in the
complexity (as `qsort` was called).
With this PR, the neighbors are always sorted at no extra cost. It is
done by appending to the neighbors list the vertices in the correct
order so the call to `qsort` is not needed anymore.

This PR partly reverts sagemath#38198 and mostly reverts sagemath#37662

### 📝 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.
- [ ] 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#38427
Reported by: cyrilbouvier
Reviewer(s): cyrilbouvier, David Coudert
@cyrilbouvier cyrilbouvier mentioned this pull request Sep 16, 2024
Merged
5 tasks
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 19, 2024
sagemathgh-38664: graphs: make init_short_digraph always sort neighbo…
04b5114 
…rs but without the extra log complexity (2nd try)

    
<!-- ^ 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 is based on the closed PR sagemath#38427 (by myself) that was breaking
too much doctests.

This PR improve the `init_short_digraph` function that is used to
initialize `StaticSparseCGraph` (used for immutable `Graph` and
`DiGraph`).

Before, a boolean parameter `sort_neighbors` was used to specify if we
wanted to sort the neighbors or not. It implied an extra `log` in the
complexity (as `qsort` was called).
With this PR, the neighbors are always sorted at no extra cost. It is
done by appending to the neighbors list the vertices in the correct
order so the call to `qsort` is not needed anymore.

This PR partly reverts sagemath#38198 and mostly reverts sagemath#37662


Contrary to PR sagemath#38427, I did not include the patch that remove the
sorting of vertices when StaticSparseGraph are initialized because it
was breaking too many doctests. Instead I added a option to disabled
said sorting and use it in my doctests to check that the new code works.

### 📝 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.
- [ ] 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#38664
Reported by: cyrilbouvier
Reviewer(s): David Coudert
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 22, 2024
sagemathgh-38664: graphs: make init_short_digraph always sort neighbo…
69845c0 
…rs but without the extra log complexity (2nd try)

    
<!-- ^ 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 is based on the closed PR sagemath#38427 (by myself) that was breaking
too much doctests.

This PR improve the `init_short_digraph` function that is used to
initialize `StaticSparseCGraph` (used for immutable `Graph` and
`DiGraph`).

Before, a boolean parameter `sort_neighbors` was used to specify if we
wanted to sort the neighbors or not. It implied an extra `log` in the
complexity (as `qsort` was called).
With this PR, the neighbors are always sorted at no extra cost. It is
done by appending to the neighbors list the vertices in the correct
order so the call to `qsort` is not needed anymore.

This PR partly reverts sagemath#38198 and mostly reverts sagemath#37662


Contrary to PR sagemath#38427, I did not include the patch that remove the
sorting of vertices when StaticSparseGraph are initialized because it
was breaking too many doctests. Instead I added a option to disabled
said sorting and use it in my doctests to check that the new code works.

### 📝 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.
- [ ] 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#38664
Reported by: cyrilbouvier
Reviewer(s): David Coudert
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcoudert dcoudert dcoudert approved these changes

Assignees
No one assigned
Labels
c: documentation c: graph theory v: small
Projects
None yet
Milestone
sage-10.4
Development

Successfully merging this pull request may close these issues.
4 participants
@cyrilbouvier @dcoudert @vbraun @mkoeppe