clean generic_graph.py (part 13) - searches and constructors

[dcoudert]

Here, we clean (PEP8 and avoid .vertices / .edges) methods:

• searches: breadth_first_search, depth_first_search (lex_BFS is done in #26630)
• constructors: add_clique, add_cycle, add_path, complement, to_simple, disjoint_union, cartesian_product, tensor_product, lexicographic_product, strong_product, disjunctive_product (methods transitive_... are done in #26634)

[dcoudert]

New commits:

​08be872

trac #26679: searches and constructors

[dcoudert]

The changes are breaking the construction of graphs.EllinghamHorton54Graph(), but so far I don't understand what's going on.

In method union, if, instead of

-        G.add_vertices(self.vertices())
-        G.add_vertices(other.vertices())
-        G.add_edges(self.edges())
-        G.add_edges(other.edges())
+        G.add_vertices(self)
+        G.add_vertices(other)
+        G.add_edges(self.edge_iterator())
+        G.add_edges(other.edge_iterator())

I let the line G.add_vertices(other.vertices()), I don't have errors anymore O_o

Any suggestion on possible cause of this issue?

[dcoudert]

My understanding is the following.

• graphs.EllinghamHorton54Graph() uses twice disjoint_union, and makes assumption on the name of vertices after each operation
• disjoint_union relabels the vertices of the graphs according the ordering list(G), and then calls union.
• When adding vertices in the graph G resulting from the union of self and other, the order in which vertices are added to G fixes the ordering list(G). So, a later call to disjoint_union, that relabels the graphs using the ordering list(G) might result in a different result.

So, either we decide that the proposed changes, avoiding to use .vertices(), is ok and then I have to update method EllinghamHorton54Graph according the new ordering, or it is not ok and we must let G.add_vertices(self.vertices()) and G.add_vertices(other.vertices()) in union.

In my opinion, the current situation in which disjoint_union relies on the ordering list(G) and union relies on the ordering G.vertices() is not good.

I can open a specific ticket if needed.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​4b12167	trac #26679: Merged with 8.5.beta6
​2eb3116	trac #26679: revert modifications of union

[dcoudert]

I have reverted modifications of method union and created a dedicated ticket (#26851).

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​2f1e2c0

trac #26679: Merged with 8.7.beta0

[dcoudert]

no merge conflict with 8.7.beta3.

[vklein]

             sage: if product_size != expected:
-            ....:     print("Something is really wrong here...", product_size, "!=", expected)
+            ....:     raise ValueError("something is really wrong here... {} != {}".format(product_size, expected))

I am not sure why raising a ValueError? would be better here ?
And to begin with the message has little value. In similar cases i think we usually do something like:

sage: product_size == expected
True

This comment is more a question and not a blocking point.

Some pep8 comments :

• 1. Multiple space after keyword :

line 17058 if (neighbors is None and not isinstance(start, list) and distance is None

• 2. Blank line contains whitespace: l 17465.
• 3. Missing whitespace after ',' :

l 17890 G.add_vertices((u,v) for u in self for v in other)

l 17972 G.add_vertices((u,v) ...

l 18043 G.add_vertices((u,v) ...

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​e2784a3	trac #26679: Merged with 8.7.beta3
​5e97618	trac #26679: reviewer's comments

[dcoudert]

Right, the test is much better like that. Thank you.

[vklein]

You're welcome.

Looks good to me.