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

Deprecate sorting by default in connected component methods to avoid type errors #35889

Closed
2 tasks done
dcoudert opened this issue Jul 3, 2023 · 6 comments · Fixed by #35891
Closed
2 tasks done

Deprecate sorting by default in connected component methods to avoid type errors #35889

dcoudert opened this issue Jul 3, 2023 · 6 comments · Fixed by #35891

Comments

@dcoudert
Copy link
Contributor

dcoudert commented Jul 3, 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.

Did you read the documentation and troubleshoot guide?

  • I have read the documentation and troubleshoot guide

Environment

- **OS**: all
- **Sage Version**: 10.1.beta5

Steps To Reproduce

This issue was reported in https://groups.google.com/g/sage-devel/c/grTffw3S15E for feedback_vertex_set but is actually located in connected_component_containing_vertex

G = Graph([("A",1)])
G.connected_component_containing_vertex(1)

Expected Behavior

['A', 1]

Actual Behavior

sage: G.connected_component_containing_vertex(1)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In [23], line 1
----> 1 G.connected_component_containing_vertex(Integer(1))

File ~/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 'str' and 'int'

Additional Information

We should deprecate sorting vertices by default in connected_components and connected_component_containing_vertex to avoid type errors when vertices are of incomparable types. Users can pass a sorting key to deal with vertices of different types.

@jhpalmieri
Copy link
Member

Something like

diff --git a/src/sage/graphs/connectivity.pyx b/src/sage/graphs/connectivity.pyx
index 0cbcb87dd8d..a2f8cb8994d 100644
--- a/src/sage/graphs/connectivity.pyx
+++ b/src/sage/graphs/connectivity.pyx
@@ -119,7 +119,7 @@ def is_connected(G):
         return len(conn_verts) == G.num_verts()
 
 
-def connected_components(G, sort=True):
+def connected_components(G, sort=False):
     """
     Return the list of connected components.
 

?

@dcoudert
Copy link
Contributor Author

dcoudert commented Jul 3, 2023

That's the idea but we have to change other parts to really fix the issue.

@videlec
Copy link
Contributor

videlec commented Jul 4, 2023

Maybe this could be fixed together with #35897?

@videlec
Copy link
Contributor

videlec commented Jul 4, 2023

And possibly, add a battery of tests for graphs without sortable vertices...

@dcoudert
Copy link
Contributor Author

dcoudert commented Jul 4, 2023

I'm currently working on deprecating sorting by default in connected_components and connected_component_containing_vertex. This is, I think, the best option. Then, if a user wants to sort vertices with incomparable types, this user can design a specific method to order vertices (i.e. the sorting key).

@dcoudert dcoudert changed the title Bug in graph method connected_component_containing_vertex when vertex labels are not comparable Deprecate sorting by default in connected component methods to avoid type errors Jul 4, 2023
@dcoudert
Copy link
Contributor Author

dcoudert commented Jul 4, 2023

I change the title and description of this issue to better reflect it's goal.

vbraun pushed a commit that referenced this issue Jul 30, 2023
… for graphs

    
Fixes #35889.

### 📚 Description

We deprecate sorting by default in `connected_components` and
`connected_component_containing_vertex`.  The default value of parameter
`sort` was `True`. We change it to `None` to identify calls when a
deprecation warning is necessary. We also add parameter `key` so that
users can define home made sorting rules.

This deprecation is needed to avoid type errors when vertices have
labels of incomparable type.

### 📝 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
    
URL: #35891
Reported by: David Coudert
Reviewer(s): Dima Pasechnik
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants