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

Meta-ticket: Audit/fix graph methods failing when vertices or edges are of incomparable types #35902

Open
1 task done
dcoudert opened this issue Jul 5, 2023 · 9 comments
Open
1 task done

Comments

@dcoudert
Copy link
Contributor

dcoudert commented Jul 5, 2023

Is there an existing issue for this?

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.

Problem Description

Several methods are failing when the vertices of the graph are of incomparable type. For instance:

sage: Graph([('A','B'),(1,2)]).edges()
<ipython-input-27-266d02e19563>:1: DeprecationWarning: parameter 'sort' will be set to False by default in the future
See https://github.com/sagemath/sage/issues/27408 for details.
  Graph([('A','B'),(Integer(1),Integer(2))]).edges()
<repr(<sage.graphs.views.EdgesView at 0x156af0280>) failed: TypeError: unsupported operand parent(s) for <: 'Integer Ring' and '<class 'str'>'>
sage: Graph([('A', 1)]).faces()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In [26], line 1
----> 1 Graph([('A', Integer(1))]).faces()

File ~/sage/src/sage/graphs/generic_graph.py:6434, in GenericGraph.faces(self, embedding)
   6432 # Storage for face paths
   6433 faces = []
-> 6434 minedge = min(edgeset)
   6435 path = [minedge]
   6436 edgeset.discard(minedge)

TypeError: '<' not supported between instances of 'int' and 'str'
sage: Graph([("A", 1)]).feedback_vertex_set()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In [2], line 1
----> 1 Graph([("A", Integer(1))]).feedback_vertex_set()

File /home/dcoudert/sage/src/sage/graphs/generic_graph.py:9150, in GenericGraph.feedback_vertex_set(self, value_only, solver, verbose, constraint_generation, integrality_tolerance)
   9145     raise ValueError("the only implementation available for "
   9146                      "undirected graphs is with constraint_generation "
   9147                      "set to True")
   9149 # It would be a pity to start a LP if the graph is already acyclic
-> 9150 if ((not self.is_directed() and self.is_forest()) or
   9151         (self.is_directed() and self.is_directed_acyclic())):
   9152     if value_only:
   9153         return 0

File /home/dcoudert/sage/src/sage/graphs/graph.py:1639, in Graph.is_forest(self, certificate, output)
   1608 @doc_index("Graph properties")
   1609 def is_forest(self, certificate=False, output='vertex'):
   1610     """
   1611     Tests if the graph is a forest, i.e. a disjoint union of trees.
   1612 
   (...)
   1637         (False, [68, 66, 69, 67, 65])
   1638     """
-> 1639     connected_components = self.connected_components()
   1640     number_of_connected_components = len(connected_components)
   1641     isit = (self.order() ==
   1642             self.size() + number_of_connected_components)

File /home/dcoudert/sage/src/sage/graphs/connectivity.pyx:166, in sage.graphs.connectivity.connected_components()
    164 for v in G:
    165     if v not in seen:
--> 166         c = connected_component_containing_vertex(G, v, sort=sort)
    167         seen.update(c)
    168         components.append(c)

File /home/dcoudert/sage/src/sage/graphs/connectivity.pyx:284, in sage.graphs.connectivity.connected_component_containing_vertex()
    282 
    283     if sort:
--> 284         c.sort()
    285     return c
    286 

TypeError: '<' not supported between instances of 'int' and 'str'

Proposed Solution

Stop sorting vertices by default whenever possible and add relevant tests / documentation when necessary.

Alternatives Considered

None.

Additional Information

Related issues / PR:

Relevant discussions on sage-devel:

@dcoudert dcoudert changed the title Meta-ticket: Audit/fix graph methods failing when vertices are of incomparable types Meta-ticket: Audit/fix graph methods failing when vertices or edges are of incomparable types Jul 5, 2023
@guninski
Copy link

guninski commented Jul 5, 2023

G=Graph([('\x03', '\x05'), ('\x01', 4), ('\x06', 4), ('\x05', 0), (0, 5), ('\x04', 0), (1, 2), (1, 4), ('\x04', 1), ('\x05', 2), ('\x02', 2), ('\x03', 3), ('\x01', 3), ('\x06', 3), ('\x03', 5), ('\x04', 5), ('\x01', '\x02'), ('\x02', '\x06')])
r=G.is_perfect()

TypeError Traceback (most recent call last)
Cell In [1], line 2

@guninski
Copy link

guninski commented Jul 5, 2023

This is with different traceback from the is_perfect() testcase

G=graphs.CompleteGraph(4)
H=Graph([("A",1)])
G.subgraph_search(H)

11369 if sort:
> 11370     return sorted(self.vertex_iterator(degree=degree, vertex_property=vertex_property), key=key)

@dcoudert
Copy link
Contributor Author

dcoudert commented Jul 5, 2023

Thanks for reporting. With #35904 we have:

sage: G = Graph([('\x03', '\x05'), ('\x01', 4), ('\x06', 4), ('\x05', 0), (0, 5), ('\x04', 0), (1, 2), (1, 4), ('\x04', 1), ('\x05', 2), ('\x02', 2), ('\x03', 3), ('\x01', 3), ('\x06', 3), ('\x03', 5), ('\x04', 5), ('\x01', '\x02'), ('\x02', '\x06')])
sage: G.is_perfect()
False

and

sage: G = graphs.CompleteGraph(4)
sage: H = Graph([("A",1)])
sage: G.subgraph_search(H)
Subgraph of (Complete graph): Graph on 2 vertices

@guninski
Copy link

guninski commented Jul 5, 2023

Have you checked regressions? You allow new codepaths, which were not reachable on python3.

@dcoudert
Copy link
Contributor Author

dcoudert commented Jul 5, 2023

Have you checked regressions? You allow new codepaths, which were not reachable on python3.

I'm not sure to understand your question. With #35904, we only make SubgraphSearch robust to vertex labels. So the overall code is more robust. Have a look at the code.

@guninski
Copy link

guninski commented Jul 5, 2023

I mean are you sure you don't introduce new bugs in extreme cases?

@dcoudert
Copy link
Contributor Author

dcoudert commented Jul 5, 2023

Well, previous doctests are satisfied and I added new doctests... Furthermore, the referees may ask for further changes.

@guninski
Copy link

guninski commented Jul 6, 2023

I don't have sage-latest and am working on 9.6 from fedora and check the testcases on sagecell.

Is there another place to check testcases on sage-latest?

@dcoudert
Copy link
Contributor Author

dcoudert commented Jul 6, 2023

You may use cocalc.com. It should use the last release (10.0).

vbraun pushed a commit that referenced this issue Jul 30, 2023
    
Part of #35902.

### 📚 Description

We ensure that the method operates properly even when vertices and edges
are of incomparable types.

On the way, we also avoid a call to `adjacency_matrix`.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [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.
- [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
- #12345: short description why this is a dependency
- #34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: #35904
Reported by: David Coudert
Reviewer(s): Matthias Köppe
vbraun pushed a commit that referenced this issue Jul 30, 2023
    
Part of #35902.

The method `find_hamiltonian` had multiple issues: it was not robust to
vertices with incomparable labels, it could loop forever if a vertex has
degree 1, a segfault was possible on small graphs, it was not properly
working for digraphs, etc.
We fix multiple issues and the method should now be safe.

### 📝 Checklist

- [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.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.
    
URL: #35956
Reported by: David Coudert
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this issue Aug 11, 2023
…e_boundary

    
Part of sagemath#35902.

### 📚 Description

We add parameter `key` to methods `multiple_edges` and `edge_boundary`.
This is useful when users manipulate graphs with edges of incomparable
types.

On the way, we avoid some calls to `multiple_edges` with `sort=True` in
method `is_tree`.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [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.
- [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#35903
Reported by: David Coudert
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this issue Aug 11, 2023
    
We do some minor changes in `sage/graphs/domination.py`.

The main change is the default value of parameter `work_on_copy` from
`False` to `True`. Firstly, it is safer to ensure that the input graph
is not modified by default, unless the user asks for such behavior.
Secondly, the method was actually working on a copy when the parameter
was `False` (wrong order of tests).

We also add some tests to show that the methods are robust to vertices
with incomparable labels (see sagemath#35902).


### 📝 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#35982
Reported by: David Coudert
Reviewer(s): David Coudert, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this issue Aug 12, 2023
…e_boundary

    
Part of sagemath#35902.

### 📚 Description

We add parameter `key` to methods `multiple_edges` and `edge_boundary`.
This is useful when users manipulate graphs with edges of incomparable
types.

On the way, we avoid some calls to `multiple_edges` with `sort=True` in
method `is_tree`.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [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.
- [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#35903
Reported by: David Coudert
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this issue Aug 12, 2023
    
We do some minor changes in `sage/graphs/domination.py`.

The main change is the default value of parameter `work_on_copy` from
`False` to `True`. Firstly, it is safer to ensure that the input graph
is not modified by default, unless the user asks for such behavior.
Secondly, the method was actually working on a copy when the parameter
was `False` (wrong order of tests).

We also add some tests to show that the methods are robust to vertices
with incomparable labels (see sagemath#35902).


### 📝 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#35982
Reported by: David Coudert
Reviewer(s): David Coudert, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 14, 2023
…tex labels

    
Part of sagemath#35902.

### 📚 Description

We ensure that method `min_spanning_tree` operates properly even when
vertices and edges are of incomparable types.
We are then able to simplify some code in
`src/sage/topology/simplicial_complex.py`.

### 📝 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.
- [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#36232
Reported by: David Coudert
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 16, 2023
…tex labels

    
Part of sagemath#35902.

### 📚 Description

We ensure that method `min_spanning_tree` operates properly even when
vertices and edges are of incomparable types.
We are then able to simplify some code in
`src/sage/topology/simplicial_complex.py`.

### 📝 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.
- [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#36232
Reported by: David Coudert
Reviewer(s): Frédéric Chapoton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants