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 bug in is_eulerian #35170

Merged

Conversation

dcoudert
Copy link
Contributor

πŸ“š Description

Fixes #35168.

We set parameter sort to False when calling connected_components. This fixes the bug as we avoid sorting vertices with different types.

πŸ“ Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

βŒ› Dependencies

None

@github-actions
Copy link

Documentation preview for this PR is ready! πŸŽ‰
Built with commit: d5e6262

@dcoudert
Copy link
Contributor Author

I don't think that the build & test / build error is related to this ticket.

Copy link
Member

@mezzarobba mezzarobba 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 vbraun merged commit 81bacf5 into sagemath:develop Mar 26, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Mar 26, 2023
vbraun pushed a commit that referenced this pull request Jun 3, 2023
… vertices

    
### πŸ“š Description

As illustrated by the following code, the method `adjacency_matrix` of
the GenericGraph class failed when vertices where not sortable.

```python
G = Graph()
G.add_vertices ([14, 'test'])
G.adjacency_matrix()
```

I fixed the problem by using `sort=False` instead of `sort=True` in the
line that get the list of vertices.

I did not open an issue for this bug, but it is similar to #35168 (fixed
by PR #35170)


**NOTE**
I started to write a test to cover the changes but all others test are
broken by this small commit. Indeed the adjacency matrix obtained with
the new code can have a different row/column order. I am not sure what
is the correct way to handle this situation

### πŸ“ 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! -->

- [x] I have made sure that the title is self-explanatory and the
description concisely explains the PR.
- [x] I have linked an issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### βŒ› Dependencies
    
URL: #35245
Reported by: cyrilbouvier
Reviewer(s): cyrilbouvier, David Coudert
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.

Graph.is_eulerian fails when the graph contains a connected component with non-comparable vertices
5 participants