GenericGraph.adjacency_matrix: using sort=True when getting vertices

[cyrilbouvier]

📚 Description

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

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

• 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

[github-actions]

Documentation preview for this PR is ready! 🎉
Built with commit: a150e2e

[dcoudert]

This PR is not similar to PR #35168 but to issue #29277. This is a consequence of the choice of Python 3 to raise an error when trying to sort a list of elements of different types (it was possible with Python 2).

I disagree with the solution you propose. Indeed, you get an adjacency matrix, but you don't know the mapping between the vertices and the rows / columns of the matrix.

When you have vertices of different types, the solution is to give as input an ordering of the vertices:

sage: G = Graph()
sage: G.add_vertices([14, 'test', 50])
sage: G.add_edge(14, 'test')
sage: G.adjacency_matrix(vertices=[14, 'test', 50])
[0 1 0]
[1 0 0]
[0 0 0]
sage: G.adjacency_matrix(vertices=['test', 50, 14])
[0 0 1]
[0 0 0]
[1 0 0]

Currently, all methods calling adjacency_matrix specify an ordering.

If you want to modify something, you can :

• add a doctest explaining users that passing an ordering of the vertices is necessary when working with vertex labels of different types
• improve the error message to point users to the previous point.

[codecov-commenter]

Codecov Report

Patch coverage: 97.95% and project coverage change: +0.01 🎉

Comparison is base (52a81cb) 88.57% compared to head (a150e2e) 88.58%.

❗ Current head a150e2e differs from pull request most recent head 094b500. Consider uploading reports for the commit 094b500 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35245      +/-   ##
===========================================
+ Coverage    88.57%   88.58%   +0.01%     
===========================================
  Files         2140     2140              
  Lines       397273   397415     +142     
===========================================
+ Hits        351891   352067     +176     
+ Misses       45382    45348      -34     

Impacted Files	Coverage Δ
src/sage/schemes/elliptic_curves/ell_generic.py	93.25% <66.66%> (+0.01%)	⬆️
src/sage/interfaces/tachyon.py	87.93% <90.00%> (+0.43%)	⬆️
src/sage/schemes/elliptic_curves/gal_reps.py	82.23% <90.00%> (+0.04%)	⬆️
src/sage/quadratic_forms/quadratic_form.py	90.26% <95.65%> (+0.16%)	⬆️
src/sage/graphs/generic_graph.py	89.02% <100.00%> (+0.49%)	⬆️
src/sage/modular/quasimodform/element.py	99.20% <100.00%> (+0.06%)	⬆️
src/sage/rings/qqbar.py	95.30% <100.00%> (+<0.01%)	⬆️
src/sage/schemes/affine/affine_morphism.py	90.33% <100.00%> (ø)
src/sage/schemes/elliptic_curves/BSD.py	43.75% <100.00%> (+0.21%)	⬆️
src/sage/schemes/elliptic_curves/cardinality.py	87.54% <100.00%> (+0.93%)	⬆️
... and 73 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[cyrilbouvier]

I disagree with the solution you propose. Indeed, you get an adjacency matrix, but you don't know the mapping between the vertices and the rows / columns of the matrix.

Indeed the method would be useless if we do not known the mapping 🤦

I commited the changes you suggested: a better error message in the exception, some explanation in the docstring and two tests

[dcoudert]

and don't add a useless space between TypeError and (.

[dcoudert]

LGTM.