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

Fix lex_BFS (and co.) for directed graphs #38269

Merged

Conversation

cyrilbouvier
Copy link
Contributor

@cyrilbouvier cyrilbouvier commented Jun 24, 2024

For directed graphs, the semantic of lex_BFS, lex_DFS, lex_DOWN and lex_UP methods are not consistent. In some parts of the code, directed graphs were converted into undirected graphs before the actual computation and in some other parts (in particular in the check function _is_valid_lex_BFS_order) directed graphs were explicitly considered as directed.

In this PR, the following changes are implemented:

  • always consider (and convert) directed graphs into undirected graphs for lex_* methods
  • do the same conversion in _is_valid_lex_BFS_order for consistency
  • add the following line in the documentation of the lex_* methods:
r"""
   Loops and multiple edges are ignored during the computation of <name of the method> and
   directed graphs are converted to undirected graphs.
"""
  • change some doctests: with the previous changes, methods can return a different (but still valid) order
  • set the copy of the graph to immutable

Fixes #38234

📝 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

Copy link

github-actions bot commented Jun 24, 2024

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

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.

otherwise LGTM.

src/sage/graphs/traversals.pyx Outdated Show resolved Hide resolved
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 Jul 20, 2024
    
<!-- ^ 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". -->

For directed graphs, the semantic of `lex_BFS`, `lex_DFS`, `lex_DOWN`
and `lex_UP` methods are not consistent. In some parts of the code,
directed graphs were converted into undirected graphs before the actual
computation and in some other parts (in particular in the check function
`_is_valid_lex_BFS_order`) directed graphs were explicitly considered as
directed.

In this PR, the following changes are implemented:
- always consider (and convert) directed graphs into undirected graphs
for `lex_*` methods
- do the same conversion in `_is_valid_lex_BFS_order` for consistency
- add the following line in the documentation of the `lex_*` methods:
```py
r"""
   Loops and multiple edges are ignored during the computation of <name
of the method> and
   directed graphs are converted to undirected graphs.
"""
```
- change some doctests: with the previous changes, methods can return a
different (but still valid) order
- set the copy of the graph to immutable

Fixes sagemath#38234

### 📝 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.
- [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#38269
Reported by: cyrilbouvier
Reviewer(s): cyrilbouvier, David Coudert
@dcoudert
Copy link
Contributor

I set this PR back to positive review.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 25, 2024
    
<!-- ^ 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". -->

For directed graphs, the semantic of `lex_BFS`, `lex_DFS`, `lex_DOWN`
and `lex_UP` methods are not consistent. In some parts of the code,
directed graphs were converted into undirected graphs before the actual
computation and in some other parts (in particular in the check function
`_is_valid_lex_BFS_order`) directed graphs were explicitly considered as
directed.

In this PR, the following changes are implemented:
- always consider (and convert) directed graphs into undirected graphs
for `lex_*` methods
- do the same conversion in `_is_valid_lex_BFS_order` for consistency
- add the following line in the documentation of the `lex_*` methods:
```py
r"""
   Loops and multiple edges are ignored during the computation of <name
of the method> and
   directed graphs are converted to undirected graphs.
"""
```
- change some doctests: with the previous changes, methods can return a
different (but still valid) order
- set the copy of the graph to immutable

Fixes sagemath#38234

### 📝 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.
- [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#38269
Reported by: cyrilbouvier
Reviewer(s): cyrilbouvier, David Coudert
vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 31, 2024
    
<!-- ^ 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". -->

For directed graphs, the semantic of `lex_BFS`, `lex_DFS`, `lex_DOWN`
and `lex_UP` methods are not consistent. In some parts of the code,
directed graphs were converted into undirected graphs before the actual
computation and in some other parts (in particular in the check function
`_is_valid_lex_BFS_order`) directed graphs were explicitly considered as
directed.

In this PR, the following changes are implemented:
- always consider (and convert) directed graphs into undirected graphs
for `lex_*` methods
- do the same conversion in `_is_valid_lex_BFS_order` for consistency
- add the following line in the documentation of the `lex_*` methods:
```py
r"""
   Loops and multiple edges are ignored during the computation of <name
of the method> and
   directed graphs are converted to undirected graphs.
"""
```
- change some doctests: with the previous changes, methods can return a
different (but still valid) order
- set the copy of the graph to immutable

Fixes sagemath#38234

### 📝 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.
- [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#38269
Reported by: cyrilbouvier
Reviewer(s): cyrilbouvier, David Coudert
vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 2, 2024
    
<!-- ^ 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". -->

For directed graphs, the semantic of `lex_BFS`, `lex_DFS`, `lex_DOWN`
and `lex_UP` methods are not consistent. In some parts of the code,
directed graphs were converted into undirected graphs before the actual
computation and in some other parts (in particular in the check function
`_is_valid_lex_BFS_order`) directed graphs were explicitly considered as
directed.

In this PR, the following changes are implemented:
- always consider (and convert) directed graphs into undirected graphs
for `lex_*` methods
- do the same conversion in `_is_valid_lex_BFS_order` for consistency
- add the following line in the documentation of the `lex_*` methods:
```py
r"""
   Loops and multiple edges are ignored during the computation of <name
of the method> and
   directed graphs are converted to undirected graphs.
"""
```
- change some doctests: with the previous changes, methods can return a
different (but still valid) order
- set the copy of the graph to immutable

Fixes sagemath#38234

### 📝 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.
- [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#38269
Reported by: cyrilbouvier
Reviewer(s): cyrilbouvier, David Coudert
@vbraun vbraun merged commit d52ed60 into sagemath:develop Aug 3, 2024
20 checks passed
@mkoeppe mkoeppe added this to the sage-10.5 milestone Aug 3, 2024
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.

lex_BFS method of DiGraph is not well-defined (the method could compute ordering that fails doctests)
4 participants