Expected ValueError when calling set_vertex()

[Rithesh17]

This is the issue discussed in the comments section of the ticket #14708:

sage: g = Graph()
sage: g.set_vertex('foo', 'bar')
sage: g.get_vertices()
{}

We should have expected a ValueError in the set_vertex() method. Also, on continuing,

sage: g.add_vertex('foo')
sage: g.get_vertices()
{'foo': 'bar'}

Which shouldn't have happened

Component: graph theory

Author: Rithesh K

Branch/Commit: 94ab6e8

Reviewer: David Coudert

Issue created by migration from https://trac.sagemath.org/ticket/27399

[Rithesh17]

New commits:

0ff156f	Polyhedron documen modified
bba6a4a	Polyhedron document modified.
32c1045	set_vertices module in generic_graph.py modified

[Rithesh17]

Branch: u/gh-Rithesh17/set_vertices-modified

[Rithesh17]

Commit: 32c1045

[fchapoton]

comment:2

The branch is not clean, but it is mixed up with another branch.

[Rithesh17]

New commits:

8ac6371

set_vertex method in generic_graph.py modified

[Rithesh17]

Changed commit from 32c1045 to 8ac6371

[Rithesh17]

Changed branch from u/gh-Rithesh17/set_vertices-modified to u/gh-Rithesh17/set_vertex-modified

[dcoudert]

comment:4

Welcome to Sagemath!

The following changes are needed in your patch:

• don't use not self._backend.has_vertex(vertex) but not self.has_vertex(vertex) or vertex not in self
• the error message is too long. You can reduce it as in method delete_vertex.
• the error message is for vertex 4 in the doctest, not 1
  so you must do:

-            ValueError: vertex (1) not in the graph.
-            Please use add_vertex() method to add the vertex to the graph before setting it to the object.
+            ValueError: vertex (4) not in the graph

and

-        if not self._backend.has_vertex(vertex):
-            raise ValueError('vertex (%s) not in the graph.\nPlease use add_vertex() method to add the vertex to the graph before setting it to the object.'%str(vertex))
+        if vertex not in self:
+            raise ValueError('vertex (%s) not in the graph'%str(vertex))

---

To check that the changes you do are OK, you must run the doctests using for instance

sage -btp --long src/sage/graphs/generic_graph.py

[Rithesh17]

Changed branch from u/gh-Rithesh17/set_vertex-modified to u/gh-Rithesh17/set_vertex-mod

[Rithesh17]

Changed commit from 8ac6371 to dae0b72

[Rithesh17]

comment:5

Thank you for the review. I have adopted all the suggestions provided.

I ran the doctests and this was the output:

Running doctests with ID 2019-03-04-00-34-10-23cf6891.
Git branch: set_vertex-mod
Using --optional=dochtml,gfortran,memlimit,mpir,python2,sage
Doctesting 1 file using 4 threads.
sage -t --long src/sage/graphs/generic_graph.py
    [3288 tests, 37.88 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 38.8 seconds
    cpu time: 32.1 seconds
    cumulative wall time: 37.9 seconds

---

New commits:

feec578	set_vertex method modified
dae0b72	set_vertex method modified

[dcoudert]

comment:6

A minor issue: remove the . at the end of 'vertex (%s) not in the graph.' and of course in the doctest.

This is PEP8.

[sagetrac-git]

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

8ac6371	set_vertex method in generic_graph.py modified
6582adb	set_vertex method modified
34b1f60	set_vertex method modified

[sagetrac-git]

Changed commit from dae0b72 to 34b1f60

[dcoudert]

Reviewer: David Coudert

[dcoudert]

comment:8

Further small remarks:

-            sage: T.set_vertex(4, graphs.DodecahedralGraph())
+            sage: T.set_vertex(4, 'foo')

There is no need to build the graph here as we just want to show that it is not possible to set label of a non existing vertex. Running all doctests is long, so when we can illustrate an issue with a smaller/faster example, it's always better.

-            raise ValueError('vertex (%s) not in the graph'%str(vertex))
+            raise ValueError('vertex (%s) not in the graph' % str(vertex))

this is also PEP8.

[sagetrac-git]

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

9ce02e6	set_vertex method modified
94ab6e8	set_vertex method modified

[sagetrac-git]

Changed commit from 34b1f60 to 94ab6e8

[dcoudert]

comment:10

For me this patch is good to go.

[vbraun]

Changed branch from u/gh-Rithesh17/set_vertex-mod to 94ab6e8